Patch 1:
The main improvement is the refactor of the state machine MIDI parser.
It is better to read and handles states properly, even weird ones.
Patch 3:
Fix a race condition.
Patches 3-4:
Miscelaneous fixes.
Felipe F. Tonello (4):
usb: gadget: f_midi: refactor state machine
usb: gadget: f_midi: added spinlock on transmit function
usb: gadget: f_midi: remove useless midi reference from port struct
usb: gadget: f_midi: add mutex_unlock under setup_fail label
drivers/usb/gadget/function/f_midi.c | 223 ++++++++++++++++++++++-------------
1 file changed, 143 insertions(+), 80 deletions(-)
--
2.6.4
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 <[email protected]>
---
drivers/usb/gadget/function/f_midi.c | 204 ++++++++++++++++++++++-------------
1 file changed, 129 insertions(+), 75 deletions(-)
diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index fb1fe96d3215..c4b47f68e553 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,166 @@ 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;
+
+ switch (b) {
+ case 0xf8 ... 0xff:
+ /* System Real-Time Messages */
+ p[0] |= 0x0f;
+ p[1] = b;
+ next_state = port->state;
+ port->state = STATE_REAL_TIME;
+ break;
- if (b >= 0xf8) {
- f_midi_transmit_packet(req, p0 | 0x0f, b, 0, 0);
- } else if (b >= 0xf0) {
+ case 0xf7:
+ /* End of SysEx */
+ switch (port->state) {
+ case STATE_SYSEX_0:
+ p[0] |= 0x05;
+ p[1] = 0xf7;
+ next_state = STATE_FINISHED;
+ break;
+ case STATE_SYSEX_1:
+ p[0] |= 0x06;
+ p[1] = port->data[0];
+ p[2] = 0xf7;
+ next_state = STATE_FINISHED;
+ break;
+ case STATE_SYSEX_2:
+ p[0] |= 0x07;
+ p[1] = port->data[0];
+ p[2] = port->data[1];
+ p[3] = 0xf7;
+ next_state = STATE_FINISHED;
+ break;
+ default:
+ /* Ignore byte */
+ next_state = port->state;
+ port->state = STATE_INITIAL;
+ }
+ break;
+
+ case 0xf0 ... 0xf6:
+ /* System Common Messages */
+ port->data[0] = port->data[1] = 0;
+ port->state = STATE_INITIAL;
switch (b) {
case 0xf0:
port->data[0] = b;
- port->state = STATE_SYSEX_1;
+ port->data[1] = 0;
+ 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) {
+ break;
+
+ case 0x80 ... 0xef:
+ /*
+ * Channel Voice Messages, Channel Mode Messages
+ * and Control Change Messages.
+ */
port->data[0] = b;
+ port->data[1] = 0;
+ port->state = STATE_INITIAL;
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;
+ break;
+
+ case 0x00 ... 0x7f:
+ /* 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);
+ if (port->data[0] < 0xf0)
+ p[0] |= port->data[0] >> 4;
+ else
+ p[0] |= 0x02;
+
+ p[1] = port->data[0];
+ p[2] = b;
+ /* This is to allow Running State Messages */
+ next_state = STATE_1PARAM;
break;
case STATE_2PARAM_1:
port->data[1] = b;
- port->state = STATE_2PARAM_2;
+ next_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);
+ 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;
+ /* This is to allow Running State Messages */
+ next_state = STATE_2PARAM_1;
break;
case STATE_SYSEX_0:
port->data[0] = b;
- port->state = STATE_SYSEX_1;
+ next_state = STATE_SYSEX_1;
break;
case STATE_SYSEX_1:
port->data[1] = b;
- port->state = STATE_SYSEX_2;
+ next_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;
+ 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 ||
+ port->state == STATE_SYSEX_2 ||
+ port->state == STATE_1PARAM ||
+ port->state == STATE_2PARAM_2 ||
+ 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 +686,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.6.4
Since f_midi_transmit is called by both ALSA and USB frameworks, it can
potentially cause a race condition between both calls. This is bad because the
way f_midi_transmit is implemented can't handle concurrent calls. This is due
to the fact that the usb request fifo looks for the next element and only if
it has data to process it enqueues the request, otherwise re-uses it. If both
(ALSA and USB) frameworks calls this function at the same time, the
kfifo_seek() will return the same usb_request, which will cause a race
condition.
To solve this problem a syncronization mechanism is necessary. In this case it
is used a spinlock since f_midi_transmit is also called by usb_request->complete
callback in interrupt context.
On benchmarks realized by me, spinlocks were more efficient then scheduling
the f_midi_transmit tasklet in process context and using a mutex to
synchronize. Also it performs better then previous implementation that
allocated a usb_request for every new transmit made.
Signed-off-by: Felipe F. Tonello <[email protected]>
---
drivers/usb/gadget/function/f_midi.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index c4b47f68e553..bbff20697f76 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -24,6 +24,7 @@
#include <linux/slab.h>
#include <linux/device.h>
#include <linux/kfifo.h>
+#include <linux/spinlock.h>
#include <sound/core.h>
#include <sound/initval.h>
@@ -98,6 +99,7 @@ struct f_midi {
/* This fifo is used as a buffer ring for pre-allocated IN usb_requests */
DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
unsigned int in_last_port;
+ spinlock_t transmit_lock;
};
static inline struct f_midi *func_to_midi(struct usb_function *f)
@@ -589,12 +591,15 @@ static void f_midi_drop_out_substreams(struct f_midi *midi)
static void f_midi_transmit(struct f_midi *midi)
{
struct usb_ep *ep = midi->in_ep;
+ unsigned long flags;
bool active;
/* We only care about USB requests if IN endpoint is enabled */
if (!ep || !ep->enabled)
goto drop_out;
+ spin_lock_irqsave(&midi->transmit_lock, flags);
+
do {
struct usb_request *req = NULL;
unsigned int len, i;
@@ -606,14 +611,17 @@ static void f_midi_transmit(struct f_midi *midi)
len = kfifo_peek(&midi->in_req_fifo, &req);
if (len != 1) {
ERROR(midi, "%s: Couldn't get usb request\n", __func__);
+ spin_unlock_irqrestore(&midi->transmit_lock, flags);
goto drop_out;
}
/* If buffer overrun, then we ignore this transmission.
* IMPORTANT: This will cause the user-space rawmidi device to block until a) usb
* requests have been completed or b) snd_rawmidi_write() times out. */
- if (req->length > 0)
+ if (req->length > 0) {
+ spin_unlock_irqrestore(&midi->transmit_lock, flags);
return;
+ }
for (i = midi->in_last_port; i < MAX_PORTS; i++) {
struct gmidi_in_port *port = midi->in_port[i];
@@ -665,6 +673,8 @@ static void f_midi_transmit(struct f_midi *midi)
}
} while (active);
+ spin_unlock_irqrestore(&midi->transmit_lock, flags);
+
return;
drop_out:
@@ -1270,6 +1280,8 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
if (status)
goto setup_fail;
+ spin_lock_init(&midi->transmit_lock);
+
++opts->refcnt;
mutex_unlock(&opts->lock);
--
2.6.4
Signed-off-by: Felipe F. Tonello <[email protected]>
---
drivers/usb/gadget/function/f_midi.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index bbff20697f76..b7d3f5a10bf0 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -70,7 +70,6 @@ enum {
* USB <- IN endpoint <- rawmidi
*/
struct gmidi_in_port {
- struct f_midi *midi;
int active;
uint8_t cable;
uint8_t state;
@@ -1256,7 +1255,6 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
goto setup_fail;
}
- port->midi = midi;
port->active = 0;
port->cable = i;
midi->in_port[i] = port;
--
2.6.4
This ensures that at any point on the function if a goto to setup_fail is
made, it will unlock the mutex.
Signed-off-by: Felipe F. Tonello <[email protected]>
---
drivers/usb/gadget/function/f_midi.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index b7d3f5a10bf0..1255f8a898d0 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -1251,7 +1251,6 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
if (!port) {
status = -ENOMEM;
- mutex_unlock(&opts->lock);
goto setup_fail;
}
@@ -1264,7 +1263,6 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
midi->id = kstrdup(opts->id, GFP_KERNEL);
if (opts->id && !midi->id) {
status = -ENOMEM;
- mutex_unlock(&opts->lock);
goto setup_fail;
}
midi->in_ports = opts->in_ports;
@@ -1293,6 +1291,7 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
return &midi->func;
setup_fail:
+ mutex_unlock(&opts->lock);
for (--i; i >= 0; i--)
kfree(midi->in_port[i]);
kfree(midi);
--
2.6.4