[PATCH v2] usb: gadget: f_midi: refactor state machine

[PATCH v2] usb: gadget: f_midi: refactor state machine

[Felipe F. Tonello]

This refactor results in a cleaner state machine code and as a result fixed a
bug when packaging a MIDI-USB packet right after a non-conformant MIDI byte stream.

Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
---
 drivers/usb/gadget/function/f_midi.c | 205 +++++++++++++++++++++--------------
 1 file changed, 124 insertions(+), 81 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index fb1fe96d3215..ab0fbeb3ee39 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -50,6 +50,19 @@ static const char f_midi_longname[] = "MIDI Gadget";
  */
 #define MAX_PORTS 16
 
+/* MIDI message states */
+enum {
+	STATE_INITIAL = 0,	/* pseudo state */
+	STATE_1PARAM,
+	STATE_2PARAM_1,
+	STATE_2PARAM_2,
+	STATE_SYSEX_0,
+	STATE_SYSEX_1,
+	STATE_SYSEX_2,
+	STATE_REAL_TIME,
+	STATE_FINISHED,	/* pseudo state */
+};
+
 /*
  * This is a gadget, and the IN/OUT naming is from the host's perspective.
  * USB -> OUT endpoint -> rawmidi
@@ -60,13 +73,6 @@ struct gmidi_in_port {
 	int active;
 	uint8_t cable;
 	uint8_t state;
-#define STATE_UNKNOWN	0
-#define STATE_1PARAM	1
-#define STATE_2PARAM_1	2
-#define STATE_2PARAM_2	3
-#define STATE_SYSEX_0	4
-#define STATE_SYSEX_1	5
-#define STATE_SYSEX_2	6
 	uint8_t data[2];
 };
 
@@ -400,118 +406,155 @@ static int f_midi_snd_free(struct snd_device *device)
 	return 0;
 }
 
-static void f_midi_transmit_packet(struct usb_request *req, uint8_t p0,
-					uint8_t p1, uint8_t p2, uint8_t p3)
-{
-	unsigned length = req->length;
-	u8 *buf = (u8 *)req->buf + length;
-
-	buf[0] = p0;
-	buf[1] = p1;
-	buf[2] = p2;
-	buf[3] = p3;
-	req->length = length + 4;
-}
-
 /*
  * Converts MIDI commands to USB MIDI packets.
  */
 static void f_midi_transmit_byte(struct usb_request *req,
 				 struct gmidi_in_port *port, uint8_t b)
 {
-	uint8_t p0 = port->cable << 4;
+	uint8_t p[4] = { port->cable << 4, 0, 0, 0 };
+	uint8_t next_state = STATE_INITIAL;
 
 	if (b >= 0xf8) {
-		f_midi_transmit_packet(req, p0 | 0x0f, b, 0, 0);
+		/* System Real-Time Messages */
+		p[0] |= 0x0f;
+		p[1] = b;
+		next_state = port->state;
+		port->state = STATE_REAL_TIME;
 	} else if (b >= 0xf0) {
+		/* System Common Messages */
+		port->data[0] = port->data[1] = 0;
 		switch (b) {
 		case 0xf0:
 			port->data[0] = b;
-			port->state = STATE_SYSEX_1;
+			next_state = STATE_SYSEX_1;
 			break;
 		case 0xf1:
 		case 0xf3:
 			port->data[0] = b;
-			port->state = STATE_1PARAM;
+			next_state = STATE_1PARAM;
 			break;
 		case 0xf2:
 			port->data[0] = b;
-			port->state = STATE_2PARAM_1;
+			next_state = STATE_2PARAM_1;
 			break;
 		case 0xf4:
 		case 0xf5:
-			port->state = STATE_UNKNOWN;
+			next_state = STATE_INITIAL;
 			break;
 		case 0xf6:
-			f_midi_transmit_packet(req, p0 | 0x05, 0xf6, 0, 0);
-			port->state = STATE_UNKNOWN;
-			break;
-		case 0xf7:
-			switch (port->state) {
-			case STATE_SYSEX_0:
-				f_midi_transmit_packet(req,
-					p0 | 0x05, 0xf7, 0, 0);
-				break;
-			case STATE_SYSEX_1:
-				f_midi_transmit_packet(req,
-					p0 | 0x06, port->data[0], 0xf7, 0);
-				break;
-			case STATE_SYSEX_2:
-				f_midi_transmit_packet(req,
-					p0 | 0x07, port->data[0],
-					port->data[1], 0xf7);
-				break;
-			}
-			port->state = STATE_UNKNOWN;
+			p[0] |= 0x05;
+			p[1] = 0xf6;
+			next_state = STATE_FINISHED;
 			break;
 		}
 	} else if (b >= 0x80) {
+		/*
+		 * Channel Voice Messages, Channel Mode Messages
+		 * and Control Change Messages.
+		 */
 		port->data[0] = b;
+		port->data[1] = 0;
 		if (b >= 0xc0 && b <= 0xdf)
-			port->state = STATE_1PARAM;
+			next_state = STATE_1PARAM;
 		else
-			port->state = STATE_2PARAM_1;
-	} else { /* b < 0x80 */
+			next_state = STATE_2PARAM_1;
+	} else {
+		/* Running message parameters */
 		switch (port->state) {
 		case STATE_1PARAM:
-			if (port->data[0] < 0xf0) {
-				p0 |= port->data[0] >> 4;
-			} else {
-				p0 |= 0x02;
-				port->state = STATE_UNKNOWN;
-			}
-			f_midi_transmit_packet(req, p0, port->data[0], b, 0);
-			break;
 		case STATE_2PARAM_1:
-			port->data[1] = b;
-			port->state = STATE_2PARAM_2;
-			break;
 		case STATE_2PARAM_2:
-			if (port->data[0] < 0xf0) {
-				p0 |= port->data[0] >> 4;
-				port->state = STATE_2PARAM_1;
-			} else {
-				p0 |= 0x03;
-				port->state = STATE_UNKNOWN;
-			}
-			f_midi_transmit_packet(req,
-				p0, port->data[0], port->data[1], b);
-			break;
 		case STATE_SYSEX_0:
-			port->data[0] = b;
-			port->state = STATE_SYSEX_1;
-			break;
 		case STATE_SYSEX_1:
-			port->data[1] = b;
-			port->state = STATE_SYSEX_2;
-			break;
-		case STATE_SYSEX_2:
-			f_midi_transmit_packet(req,
-				p0 | 0x04, port->data[0], port->data[1], b);
-			port->state = STATE_SYSEX_0;
+		case STATE_SYSEX_2: {
+			if (b == 0xf7) {
+				/* End of SysEx */
+				switch (port->state) {
+				case STATE_SYSEX_0:
+					p[0] |= 0x05;
+					p[1] = 0xf7;
+					break;
+				case STATE_SYSEX_1:
+					p[0] |= 0x06;
+					p[1] = port->data[0];
+					p[2] = 0xf7;
+					break;
+				case STATE_SYSEX_2:
+					p[0] |= 0x07;
+					p[1] = port->data[0];
+					p[2] = port->data[1];
+					p[3] = 0xf7;
+					break;
+				}
+				next_state = STATE_FINISHED;
+			} else if (b < 0x80) {
+				/* Message parameters */
+				switch (port->state) {
+				case STATE_1PARAM:
+					if (port->data[0] < 0xf0)
+						p[0] |= port->data[0] >> 4;
+					else
+						p[0] |= 0x02;
+
+					p[1] = port->data[0];
+					p[2] = b;
+					next_state = STATE_FINISHED;
+					break;
+				case STATE_2PARAM_1:
+					port->data[1] = b;
+					next_state = STATE_2PARAM_2;
+					break;
+				case STATE_2PARAM_2:
+					if (port->data[0] < 0xf0)
+						p[0] |= port->data[0] >> 4;
+					else
+						p[0] |= 0x03;
+
+					p[1] = port->data[0];
+					p[2] = port->data[1];
+					p[3] = b;
+					next_state = STATE_FINISHED;
+					break;
+				case STATE_SYSEX_0:
+					port->data[0] = b;
+					next_state = STATE_SYSEX_1;
+					break;
+				case STATE_SYSEX_1:
+					port->data[1] = b;
+					next_state = STATE_SYSEX_2;
+					break;
+				case STATE_SYSEX_2:
+					p[0] |= 0x04;
+					p[1] = port->data[0];
+					p[2] = port->data[1];
+					p[3] = b;
+					next_state = STATE_SYSEX_0;
+					break;
+				}
+			}
 			break;
 		}
+		}
+	}
+
+	/* States where we have to write into the USB request */
+	if (next_state == STATE_FINISHED ||
+	    next_state == STATE_SYSEX_0 ||
+	    port->state == STATE_REAL_TIME) {
+		unsigned length = req->length;
+		u8 *buf = (u8 *)req->buf + length;
+
+		memcpy(buf, p, sizeof(p));
+		req->length = length + sizeof(p);
+
+		if (next_state == STATE_FINISHED)
+			next_state = STATE_INITIAL;
+
+		port->data[0] = port->data[1] = 0;
 	}
+
+	port->state = next_state;
 }
 
 static void f_midi_drop_out_substreams(struct f_midi *midi)
@@ -632,7 +675,7 @@ static int f_midi_in_open(struct snd_rawmidi_substream *substream)
 
 	VDBG(midi, "%s()\n", __func__);
 	midi->in_substream[substream->number] = substream;
-	midi->in_port[substream->number]->state = STATE_UNKNOWN;
+	midi->in_port[substream->number]->state = STATE_INITIAL;
 	return 0;
 }
 
-- 
2.5.0

Re: [PATCH v2] usb: gadget: f_midi: refactor state machine

[Felipe Ferreri Tonello]

[-- Attachment #1: Type: text/plain, Size: 423 bytes --]

Hi Balbi,

On 23/12/15 12:58, Felipe F. Tonello wrote:
> This refactor results in a cleaner state machine code and as a result fixed a
> bug when packaging a MIDI-USB packet right after a non-conformant MIDI byte stream.
> 
> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>

Please disconsider this patch. Instead consider the patch in the "[PATCH
0/4] More improvements on MIDI gadget function" patch set.

Felipe

[-- Attachment #2: 0x92698E6A.asc --]
[-- Type: application/pgp-keys, Size: 7195 bytes --]