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

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

[Alyandra Zahran Nadhir]

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>
---
 sound/core/ump_convert.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/sound/core/ump_convert.c b/sound/core/ump_convert.c
index 0a7972d91..b112064cb 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)
@@ -385,6 +386,7 @@ static int cvt_legacy_cmd_to_ump(struct ump_cvt_to_ump *cvt,
 			return ret;
 		case UMP_CC_RPN_LSB:
 			ret = fill_rpn(cc, midi2, true);
+			cc->nrpn_set = 0;
 			cc->rpn_set = 1;
 			cc->cc_rpn_lsb = buf[2];
 			if (cc->cc_rpn_msb == 0x7f && cc->cc_rpn_lsb == 0x7f)
@@ -392,11 +394,13 @@ static int cvt_legacy_cmd_to_ump(struct ump_cvt_to_ump *cvt,
 			return ret;
 		case UMP_CC_NRPN_MSB:
 			ret = fill_rpn(cc, midi2, true);
+			cc->rpn_set = 0;
 			cc->nrpn_set = 1;
 			cc->cc_nrpn_msb = buf[2];
 			return ret;
 		case UMP_CC_NRPN_LSB:
 			ret = fill_rpn(cc, midi2, true);
+			cc->rpn_set = 0;
 			cc->nrpn_set = 1;
 			cc->cc_nrpn_lsb = buf[2];
 			return ret;

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

[Alyandra Zahran Nadhir]

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>
---
 sound/core/seq/Kconfig           |  12 ++
 sound/core/seq/seq_ump_convert.c | 275 +++++++++++++++++++++++++++++++
 2 files changed, 287 insertions(+)

diff --git a/sound/core/seq/Kconfig b/sound/core/seq/Kconfig
index 0d6061591..8c0888890 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
+	help
+	  Tests RPN/NRPN selector transitions through the Controller and
+	  Control14 sequencer event paths. It covers pending-data flushes,
+	  null-RPN reset, per-channel state isolation, and legacy MIDI 1.0
+	  protocol conversion. The suite requires no MIDI hardware.
+
 endif # SND_SEQUENCER
diff --git a/sound/core/seq/seq_ump_convert.c b/sound/core/seq/seq_ump_convert.c
index ff4ee26ad..5e4071130 100644
--- a/sound/core/seq/seq_ump_convert.c
+++ b/sound/core/seq/seq_ump_convert.c
@@ -6,6 +6,10 @@
 #include <linux/init.h>
 #include <linux/errno.h>
 #include <linux/string.h>
+#if IS_ENABLED(CONFIG_SND_SEQ_UMP_KUNIT_TEST)
+#include <kunit/test.h>
+#endif
+#include <sound/asound.h>
 #include <sound/core.h>
 #include <sound/ump.h>
 #include <sound/ump_msg.h>
@@ -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)
@@ -854,6 +859,7 @@ static int cc_ev_to_ump_midi2(const struct snd_seq_event *event,
 		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)
@@ -861,11 +867,13 @@ static int cc_ev_to_ump_midi2(const struct snd_seq_event *event,
 		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;
@@ -975,6 +983,7 @@ static int ctrl14_ev_to_ump_midi2(const struct snd_seq_event *event,
 	case UMP_CC_RPN_MSB:
 	case UMP_CC_RPN_LSB:
 		ret = fill_rpn(cc, data, channel, true);
+		cc->nrpn_set = 0;
 		cc->cc_rpn_msb = msb;
 		cc->cc_rpn_lsb = lsb;
 		cc->rpn_set = 1;
@@ -984,6 +993,7 @@ static int ctrl14_ev_to_ump_midi2(const struct snd_seq_event *event,
 	case UMP_CC_NRPN_MSB:
 	case UMP_CC_NRPN_LSB:
 		ret = fill_rpn(cc, data, channel, true);
+		cc->rpn_set = 0;
 		cc->cc_nrpn_msb = msb;
 		cc->nrpn_set = 1;
 		cc->cc_nrpn_lsb = lsb;
@@ -1310,3 +1320,268 @@ int snd_seq_ump_group_port(const struct snd_seq_event *event)
 	/* group-port number starts from 1 */
 	return ump_message_group(ump_ev->ump[0]) + 1;
 }
+
+#if IS_ENABLED(CONFIG_SND_SEQ_UMP_KUNIT_TEST)
+static void seq_ump_kunit_set_cc(struct snd_seq_event *event,
+				 unsigned char channel, unsigned char index,
+				 int value)
+{
+	memset(event, 0, sizeof(*event));
+	event->type = SNDRV_SEQ_EVENT_CONTROLLER;
+	event->data.control.channel = channel;
+	event->data.control.param = index;
+	event->data.control.value = value;
+}
+
+static void seq_ump_kunit_set_ctrl14(struct snd_seq_event *event,
+				     unsigned char channel,
+				     unsigned char index, int value)
+{
+	memset(event, 0, sizeof(*event));
+	event->type = SNDRV_SEQ_EVENT_CONTROL14;
+	event->data.control.channel = channel;
+	event->data.control.param = index;
+	event->data.control.value = value;
+}
+
+static void seq_ump_cc_selector_switch_test(struct kunit *test)
+{
+	struct snd_seq_client_port *port;
+	struct snd_seq_event event;
+	union snd_ump_midi2_msg data = {};
+
+	port = kunit_kzalloc(test, sizeof(*port), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, port);
+
+	seq_ump_kunit_set_cc(&event, 0, UMP_CC_RPN_MSB, 0x11);
+	KUNIT_EXPECT_EQ(test, cc_ev_to_ump_midi2(&event, port, &data,
+						 UMP_MSG_STATUS_CC), 0);
+	seq_ump_kunit_set_cc(&event, 0, UMP_CC_RPN_LSB, 0x12);
+	KUNIT_EXPECT_EQ(test, cc_ev_to_ump_midi2(&event, port, &data,
+						 UMP_MSG_STATUS_CC), 0);
+	seq_ump_kunit_set_cc(&event, 0, UMP_CC_NRPN_MSB, 0x21);
+	KUNIT_EXPECT_EQ(test, cc_ev_to_ump_midi2(&event, port, &data,
+						 UMP_MSG_STATUS_CC), 0);
+	KUNIT_EXPECT_FALSE(test, port->midi2_bank[0].rpn_set);
+	KUNIT_EXPECT_TRUE(test, port->midi2_bank[0].nrpn_set);
+	seq_ump_kunit_set_cc(&event, 0, UMP_CC_NRPN_LSB, 0x22);
+	KUNIT_EXPECT_EQ(test, cc_ev_to_ump_midi2(&event, port, &data,
+						 UMP_MSG_STATUS_CC), 0);
+	seq_ump_kunit_set_cc(&event, 0, UMP_CC_DATA, 0x31);
+	KUNIT_EXPECT_EQ(test, cc_ev_to_ump_midi2(&event, port, &data,
+						 UMP_MSG_STATUS_CC), 0);
+	seq_ump_kunit_set_cc(&event, 0, UMP_CC_DATA_LSB, 0x32);
+	KUNIT_EXPECT_EQ(test, cc_ev_to_ump_midi2(&event, port, &data,
+						 UMP_MSG_STATUS_CC), 1);
+	KUNIT_EXPECT_EQ(test, (unsigned int)data.rpn.status, UMP_MSG_STATUS_NRPN);
+	KUNIT_EXPECT_EQ(test, (unsigned int)data.rpn.bank, (unsigned char)0x21);
+	KUNIT_EXPECT_EQ(test, (unsigned int)data.rpn.index, (unsigned char)0x22);
+
+	memset(&data, 0, sizeof(data));
+	seq_ump_kunit_set_cc(&event, 1, UMP_CC_NRPN_MSB, 0x41);
+	KUNIT_EXPECT_EQ(test, cc_ev_to_ump_midi2(&event, port, &data,
+						 UMP_MSG_STATUS_CC), 0);
+	seq_ump_kunit_set_cc(&event, 1, UMP_CC_NRPN_LSB, 0x42);
+	KUNIT_EXPECT_EQ(test, cc_ev_to_ump_midi2(&event, port, &data,
+						 UMP_MSG_STATUS_CC), 0);
+	seq_ump_kunit_set_cc(&event, 1, UMP_CC_RPN_MSB, 0x51);
+	KUNIT_EXPECT_EQ(test, cc_ev_to_ump_midi2(&event, port, &data,
+						 UMP_MSG_STATUS_CC), 0);
+	KUNIT_EXPECT_FALSE(test, port->midi2_bank[1].nrpn_set);
+	KUNIT_EXPECT_TRUE(test, port->midi2_bank[1].rpn_set);
+	seq_ump_kunit_set_cc(&event, 1, UMP_CC_RPN_LSB, 0x52);
+	KUNIT_EXPECT_EQ(test, cc_ev_to_ump_midi2(&event, port, &data,
+						 UMP_MSG_STATUS_CC), 0);
+	seq_ump_kunit_set_cc(&event, 1, UMP_CC_DATA, 0x61);
+	KUNIT_EXPECT_EQ(test, cc_ev_to_ump_midi2(&event, port, &data,
+						 UMP_MSG_STATUS_CC), 0);
+	seq_ump_kunit_set_cc(&event, 1, UMP_CC_DATA_LSB, 0x62);
+	KUNIT_EXPECT_EQ(test, cc_ev_to_ump_midi2(&event, port, &data,
+						 UMP_MSG_STATUS_CC), 1);
+	KUNIT_EXPECT_EQ(test, (unsigned int)data.rpn.status, UMP_MSG_STATUS_RPN);
+	KUNIT_EXPECT_EQ(test, (unsigned int)data.rpn.bank, (unsigned char)0x51);
+	KUNIT_EXPECT_EQ(test, (unsigned int)data.rpn.index, (unsigned char)0x52);
+}
+
+static void seq_ump_cc_pending_flush_and_channel_test(struct kunit *test)
+{
+	struct snd_seq_client_port *port;
+	struct snd_seq_event event;
+	union snd_ump_midi2_msg data = {};
+
+	port = kunit_kzalloc(test, sizeof(*port), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, port);
+
+	seq_ump_kunit_set_cc(&event, 2, UMP_CC_RPN_MSB, 0x09);
+	cc_ev_to_ump_midi2(&event, port, &data, UMP_MSG_STATUS_CC);
+	seq_ump_kunit_set_cc(&event, 2, UMP_CC_RPN_LSB, 0x08);
+	cc_ev_to_ump_midi2(&event, port, &data, UMP_MSG_STATUS_CC);
+	seq_ump_kunit_set_cc(&event, 3, UMP_CC_NRPN_MSB, 0x17);
+	cc_ev_to_ump_midi2(&event, port, &data, UMP_MSG_STATUS_CC);
+	seq_ump_kunit_set_cc(&event, 3, UMP_CC_NRPN_LSB, 0x18);
+	cc_ev_to_ump_midi2(&event, port, &data, UMP_MSG_STATUS_CC);
+	seq_ump_kunit_set_cc(&event, 2, UMP_CC_DATA, 0x25);
+	KUNIT_EXPECT_EQ(test, cc_ev_to_ump_midi2(&event, port, &data,
+						 UMP_MSG_STATUS_CC), 0);
+
+	seq_ump_kunit_set_cc(&event, 2, UMP_CC_NRPN_MSB, 0x29);
+	KUNIT_EXPECT_EQ(test, cc_ev_to_ump_midi2(&event, port, &data,
+						 UMP_MSG_STATUS_CC), 1);
+	KUNIT_EXPECT_EQ(test, (unsigned int)data.rpn.status, UMP_MSG_STATUS_RPN);
+	KUNIT_EXPECT_EQ(test, (unsigned int)data.rpn.bank, (unsigned char)0x09);
+	KUNIT_EXPECT_EQ(test, (unsigned int)data.rpn.index, (unsigned char)0x08);
+	KUNIT_EXPECT_EQ(test, (unsigned int)data.rpn.data, upscale_14_to_32bit(0x25 << 7));
+	KUNIT_EXPECT_FALSE(test, port->midi2_bank[2].rpn_set);
+	KUNIT_EXPECT_TRUE(test, port->midi2_bank[2].nrpn_set);
+	KUNIT_EXPECT_TRUE(test, port->midi2_bank[3].nrpn_set);
+	KUNIT_EXPECT_EQ(test, port->midi2_bank[3].cc_nrpn_msb,
+			(unsigned char)0x17);
+}
+
+static void seq_ump_ctrl14_selector_switch_test(struct kunit *test)
+{
+	struct snd_seq_client_port *port;
+	struct snd_seq_event event;
+	union snd_ump_midi2_msg data = {};
+
+	port = kunit_kzalloc(test, sizeof(*port), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, port);
+
+	seq_ump_kunit_set_ctrl14(&event, 4, UMP_CC_RPN_MSB, (1 << 7) | 2);
+	KUNIT_EXPECT_EQ(test, ctrl14_ev_to_ump_midi2(&event, port, &data,
+						     UMP_MSG_STATUS_CC), 0);
+	seq_ump_kunit_set_ctrl14(&event, 4, UMP_CC_NRPN_MSB, (3 << 7) | 4);
+	KUNIT_EXPECT_EQ(test, ctrl14_ev_to_ump_midi2(&event, port, &data,
+						     UMP_MSG_STATUS_CC), 0);
+	KUNIT_EXPECT_FALSE(test, port->midi2_bank[4].rpn_set);
+	KUNIT_EXPECT_TRUE(test, port->midi2_bank[4].nrpn_set);
+	seq_ump_kunit_set_ctrl14(&event, 4, UMP_CC_DATA, (5 << 7) | 6);
+	KUNIT_EXPECT_EQ(test, ctrl14_ev_to_ump_midi2(&event, port, &data,
+						     UMP_MSG_STATUS_CC), 1);
+	KUNIT_EXPECT_EQ(test, (unsigned int)data.rpn.status, UMP_MSG_STATUS_NRPN);
+	KUNIT_EXPECT_EQ(test, (unsigned int)data.rpn.bank, (unsigned char)3);
+	KUNIT_EXPECT_EQ(test, (unsigned int)data.rpn.index, (unsigned char)4);
+
+	memset(&data, 0, sizeof(data));
+	seq_ump_kunit_set_ctrl14(&event, 5, UMP_CC_NRPN_MSB, (7 << 7) | 8);
+	ctrl14_ev_to_ump_midi2(&event, port, &data, UMP_MSG_STATUS_CC);
+	seq_ump_kunit_set_ctrl14(&event, 5, UMP_CC_RPN_MSB, (9 << 7) | 10);
+	KUNIT_EXPECT_EQ(test, ctrl14_ev_to_ump_midi2(&event, port, &data,
+						     UMP_MSG_STATUS_CC), 0);
+	KUNIT_EXPECT_FALSE(test, port->midi2_bank[5].nrpn_set);
+	KUNIT_EXPECT_TRUE(test, port->midi2_bank[5].rpn_set);
+	seq_ump_kunit_set_ctrl14(&event, 5, UMP_CC_DATA, (11 << 7) | 12);
+	KUNIT_EXPECT_EQ(test, ctrl14_ev_to_ump_midi2(&event, port, &data,
+						     UMP_MSG_STATUS_CC), 1);
+	KUNIT_EXPECT_EQ(test, (unsigned int)data.rpn.status, UMP_MSG_STATUS_RPN);
+	KUNIT_EXPECT_EQ(test, (unsigned int)data.rpn.bank, (unsigned char)9);
+	KUNIT_EXPECT_EQ(test, (unsigned int)data.rpn.index, (unsigned char)10);
+}
+
+static void seq_ump_ctrl14_pending_flush_test(struct kunit *test)
+{
+	struct snd_seq_client_port *port;
+	struct snd_seq_event event;
+	union snd_ump_midi2_msg data = {};
+
+	port = kunit_kzalloc(test, sizeof(*port), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, port);
+
+	seq_ump_kunit_set_ctrl14(&event, 6, UMP_CC_NRPN_MSB, (2 << 7) | 3);
+	ctrl14_ev_to_ump_midi2(&event, port, &data, UMP_MSG_STATUS_CC);
+	seq_ump_kunit_set_cc(&event, 6, UMP_CC_DATA, 0x35);
+	KUNIT_EXPECT_EQ(test, cc_ev_to_ump_midi2(&event, port, &data,
+						 UMP_MSG_STATUS_CC), 0);
+
+	seq_ump_kunit_set_ctrl14(&event, 6, UMP_CC_RPN_MSB, (4 << 7) | 5);
+	KUNIT_EXPECT_EQ(test, ctrl14_ev_to_ump_midi2(&event, port, &data,
+						     UMP_MSG_STATUS_CC), 1);
+	KUNIT_EXPECT_EQ(test, (unsigned int)data.rpn.status, UMP_MSG_STATUS_NRPN);
+	KUNIT_EXPECT_EQ(test, (unsigned int)data.rpn.bank, (unsigned char)2);
+	KUNIT_EXPECT_EQ(test, (unsigned int)data.rpn.index, (unsigned char)3);
+	KUNIT_EXPECT_EQ(test, (unsigned int)data.rpn.data, upscale_14_to_32bit(0x35 << 7));
+	KUNIT_EXPECT_FALSE(test, port->midi2_bank[6].nrpn_set);
+	KUNIT_EXPECT_TRUE(test, port->midi2_bank[6].rpn_set);
+}
+
+static void seq_ump_null_rpn_test(struct kunit *test)
+{
+	struct snd_seq_client_port *port;
+	struct snd_seq_event event;
+	union snd_ump_midi2_msg data = {};
+
+	port = kunit_kzalloc(test, sizeof(*port), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, port);
+
+	seq_ump_kunit_set_cc(&event, 7, UMP_CC_NRPN_MSB, 1);
+	cc_ev_to_ump_midi2(&event, port, &data, UMP_MSG_STATUS_CC);
+	seq_ump_kunit_set_cc(&event, 7, UMP_CC_NRPN_LSB, 2);
+	cc_ev_to_ump_midi2(&event, port, &data, UMP_MSG_STATUS_CC);
+	seq_ump_kunit_set_cc(&event, 7, UMP_CC_RPN_MSB, 0x7f);
+	cc_ev_to_ump_midi2(&event, port, &data, UMP_MSG_STATUS_CC);
+	seq_ump_kunit_set_cc(&event, 7, UMP_CC_RPN_LSB, 0x7f);
+	cc_ev_to_ump_midi2(&event, port, &data, UMP_MSG_STATUS_CC);
+	KUNIT_EXPECT_FALSE(test, port->midi2_bank[7].rpn_set);
+	KUNIT_EXPECT_FALSE(test, port->midi2_bank[7].nrpn_set);
+
+	seq_ump_kunit_set_ctrl14(&event, 8, UMP_CC_NRPN_MSB, (3 << 7) | 4);
+	ctrl14_ev_to_ump_midi2(&event, port, &data, UMP_MSG_STATUS_CC);
+	seq_ump_kunit_set_ctrl14(&event, 8, UMP_CC_RPN_MSB, 0x3fff);
+	ctrl14_ev_to_ump_midi2(&event, port, &data, UMP_MSG_STATUS_CC);
+	KUNIT_EXPECT_FALSE(test, port->midi2_bank[8].rpn_set);
+	KUNIT_EXPECT_FALSE(test, port->midi2_bank[8].nrpn_set);
+}
+
+static void seq_ump_midi1_passthrough_test(struct kunit *test)
+{
+	struct snd_seq_client_port *port;
+	struct snd_seq_event event;
+	union snd_ump_midi1_msg seq_data = {};
+	struct ump_cvt_to_ump cvt;
+	static const unsigned char cc_messages[][3] = {
+		{ 0xb3, UMP_CC_RPN_MSB, 0x12 },
+		{ 0xb3, UMP_CC_NRPN_MSB, 0x34 },
+	};
+	int i, j;
+
+	port = kunit_kzalloc(test, sizeof(*port), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, port);
+	seq_ump_kunit_set_cc(&event, 9, 0x40, 0x22);
+	KUNIT_EXPECT_EQ(test, cc_ev_to_ump_midi1(&event, port, &seq_data,
+						 UMP_MSG_STATUS_CC), 1);
+	KUNIT_EXPECT_EQ(test, (unsigned int)seq_data.cc.status, UMP_MSG_STATUS_CC);
+	KUNIT_EXPECT_EQ(test, (unsigned int)seq_data.cc.channel, (unsigned char)9);
+	KUNIT_EXPECT_EQ(test, (unsigned int)seq_data.cc.index, (unsigned char)0x40);
+	KUNIT_EXPECT_EQ(test, (unsigned int)seq_data.cc.data, (unsigned char)0x22);
+
+	snd_ump_convert_reset(&cvt);
+	for (i = 0; i < ARRAY_SIZE(cc_messages); i++) {
+		for (j = 0; j < ARRAY_SIZE(cc_messages[i]); j++)
+			snd_ump_convert_to_ump(&cvt, 0,
+					       SNDRV_UMP_EP_INFO_PROTO_MIDI1,
+					       cc_messages[i][j]);
+		KUNIT_EXPECT_EQ(test, cvt.ump_bytes, 4);
+		KUNIT_EXPECT_EQ(test, ump_message_type(cvt.ump[0]),
+				UMP_MSG_TYPE_MIDI1_CHANNEL_VOICE);
+		KUNIT_EXPECT_FALSE(test, cvt.bank[3].rpn_set);
+		KUNIT_EXPECT_FALSE(test, cvt.bank[3].nrpn_set);
+	}
+}
+
+static struct kunit_case seq_ump_convert_test_cases[] = {
+	KUNIT_CASE(seq_ump_cc_selector_switch_test),
+	KUNIT_CASE(seq_ump_cc_pending_flush_and_channel_test),
+	KUNIT_CASE(seq_ump_ctrl14_selector_switch_test),
+	KUNIT_CASE(seq_ump_ctrl14_pending_flush_test),
+	KUNIT_CASE(seq_ump_null_rpn_test),
+	KUNIT_CASE(seq_ump_midi1_passthrough_test),
+	{}
+};
+
+static struct kunit_suite seq_ump_convert_test_suite = {
+	.name = "seq-ump-convert",
+	.test_cases = seq_ump_convert_test_cases,
+};
+
+kunit_test_suite(seq_ump_convert_test_suite);
+#endif

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

[sashiko-bot]

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

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

[sashiko-bot]

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

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

[Takashi Iwai]

On Sat, 07 Nov 2026 12:10:54 +0100,
Alyandra Zahran Nadhir wrote:
> 
> 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>

Thanks for the patch.

Recently there was a discussion about RPN/NRPN handling, and this
looks related:
  https://lore.kernel.org/20261104121508.3972314-1-sammiee5311@gmail.com

Adding HyeongJun to Cc.


Takashi

> ---
>  sound/core/ump_convert.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/sound/core/ump_convert.c b/sound/core/ump_convert.c
> index 0a7972d91..b112064cb 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)
> @@ -385,6 +386,7 @@ static int cvt_legacy_cmd_to_ump(struct ump_cvt_to_ump *cvt,
>  			return ret;
>  		case UMP_CC_RPN_LSB:
>  			ret = fill_rpn(cc, midi2, true);
> +			cc->nrpn_set = 0;
>  			cc->rpn_set = 1;
>  			cc->cc_rpn_lsb = buf[2];
>  			if (cc->cc_rpn_msb == 0x7f && cc->cc_rpn_lsb == 0x7f)
> @@ -392,11 +394,13 @@ static int cvt_legacy_cmd_to_ump(struct ump_cvt_to_ump *cvt,
>  			return ret;
>  		case UMP_CC_NRPN_MSB:
>  			ret = fill_rpn(cc, midi2, true);
> +			cc->rpn_set = 0;
>  			cc->nrpn_set = 1;
>  			cc->cc_nrpn_msb = buf[2];
>  			return ret;
>  		case UMP_CC_NRPN_LSB:
>  			ret = fill_rpn(cc, midi2, true);
> +			cc->rpn_set = 0;
>  			cc->nrpn_set = 1;
>  			cc->cc_nrpn_lsb = buf[2];
>  			return ret;

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

[HyeongJun An]

Hello Takashi,

Thanks for adding me, and thanks Alyandra for the patches. Yes, this is
one of the cases from that thread.

I have patches for the other cases discussed there. They keep the
RPN/NRPN selection after a send and clear only the data, drop data when
nothing is selected, and handle a Null NRPN like a Null RPN. They also
need the same selector change as this patch.

I can send them on top of Alyandra's patches, or as one series that
includes the selector change, whichever you prefer.

Best regards,
HyeongJun An

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

[Takashi Iwai]

On Sun, 08 Nov 2026 07:07:08 +0100,
Alyandra SigmabetJir wrote:
> 
> 
> Hi HyeongJun,
> 
> Thank you for bringing up these related cases in the earlier discussion. I
> also appreciate you taking the time to work on the additional patches.
> 
> Thank you for clarifying the situation and sharing the details.I appreciate
> your help and the work you've already put into this.

Could you guys cooperate and organize a single fix series?  That'd be
nicer at merge.

Also, about the unit test: I'm not much against kunit, but my gut
feeling is that selftest would be more flexible (and less changes in
the core code itself).  If that's feasible, let's go in that way.
Otherwise we can take the kunit change.  But maybe it's cleaner to
move the test code into another file.


thanks,

Takashi

> 
> Sincerely,
> Alyandra
> 
> Pada Sab, 7 Nov 2026 21.29, HyeongJun An <sammiee5311@gmail.com> menulis:
> 
>     Hello Takashi,
>    
>     Thanks for adding me, and thanks Alyandra for the patches. Yes, this is
>     one of the cases from that thread.
>    
>     I have patches for the other cases discussed there. They keep the
>     RPN/NRPN selection after a send and clear only the data, drop data when
>     nothing is selected, and handle a Null NRPN like a Null RPN. They also
>     need the same selector change as this patch.
>    
>     I can send them on top of Alyandra's patches, or as one series that
>     includes the selector change, whichever you prefer.
>    
>     Best regards,
>     HyeongJun An
> 

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

[HyeongJun An]

Hello Takashi and Alyandra,

Sure. I'll put Alyandra's patches first and mine on top, without the
KUnit part.

For tests, I have a sequencer test that I used for these changes, and
I can turn it into a selftest as a separate patch, with Alyandra's
KUnit cases added.

Alyandra, would that be okay with you?

Best regards,
HyeongJun An

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

[alyandra5b]

Hello HyeongJun and Takashi,

Yes, that sounds good to me.

I'm happy with putting my patches first, followed by yours, and moving the tests into a separate selftest patch.

Thanks for offering to adapt the sequencer test and incorporate my test cases. I think this would be a cleaner approach.

Best regards,
Alyandra