Re: [PATCH 1/2] usb: gadget: f_midi: refactor state machine

[Clemens Ladisch]

Felipe Ferreri Tonello wrote:
>> Running status is feature.
>
>What do you mean by that?

That this behavior is intended, and required.

> I don't qualify writing a *wrong* MIDI-USB
>packet because of a previous MIDI message as a feature.

The MIDI Specification qualifies Running Status as a feature.


Regards,
Clemens

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] usb: gadget: f_midi: refactor state machine

[Felipe Ferreri Tonello]

Hi Clemens,

On 22/12/15 17:10, Clemens Ladisch wrote:
> Felipe F. Tonello wrote:
>> This refactor includes the following:
>>  * Cleaner state machine code;
> 
> It does not correctly handle system real time messages inserted between
> the status and data bytes of other messages.

True, thanks for pointing that out. I fixed that on next revision of
this patch.

> 
>>  * Reset state if MIDI message parsed is non-conformant;
> 
> Why?  In a byte stream like "C1 C3 44", where the data byte of the first
> message was lost, the reset would also drop the second message.

True. That was fixed as well.

> 
>>  * Fixed bug when a conformant MIDI message was followed by a non-conformant
>>causing the MIDI-USB message to use old temporary data (port->data[0..1]),
>>thus packing a wrong MIDI-USB request.
> 
> Running status is feature.

What do you mean by that? I don't qualify writing a *wrong* MIDI-USB
packet because of a previous MIDI message as a feature.

For instance, try this MIDI message:
"8A 54 24 00 40"

It will be converted to MIDI-USB as "08 8A 54 24 08 8A 00 40" which is
wrong. It should only be "08 8A 54 24" and ignore the "00 40" MIDI bytes.

On every state byte the message should basically reset data[0..1] to
zero overwriting previous data. This should also be true when a MIDI-USB
packet is complete.

Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 1/2] usb: gadget: f_midi: refactor state machine

[Felipe Ferreri Tonello]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

Hi Balbi,

On 22/12/15 17:49, Felipe Balbi wrote:
> 
> Hi,
> 
> "Felipe F. Tonello"  writes:
>> This refactor includes the following: * Cleaner state machine
>> code; * Reset state if MIDI message parsed is non-conformant; *
>> Fixed bug when a conformant MIDI message was followed by a
>> non-conformant causing the MIDI-USB message to use old temporary
>> data (port->data[0..1]), thus packing a wrong MIDI-USB request.
> 
> we don't do more than one logical thing per patch. Please split
> this up.

Actually this patch has only one logical change, the state machine
refactoring. But by doing it, those three items were a consequence.

Felipe
-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iQIcBAEBCAAGBQJWemc+AAoJEMxEtNCSaY5qpXwP/1B/sVsfapRtP4dw93YF6En5
w/ej9JatIyJIaXNxauCVzgRl9uuiXGyEqErRjJdodyvHar3yKvD9HEXE6MhowEp4
JMD/phN2v9Sdj1VxRf9Z0XDSWsg6huVhfQU47HBMRGiY8ezvEgir2bvg7dYbZMsv
ACgIx8oh6N/AEHPq9GoLbEpXfJ58Pl564Sq/2o6wWsJQS06A+jp+JmqK4Y3eB5M5
18rmLW7lQLcZPO08Pf/c6BExWQLbzY/mT8KofwycvC9hWxYp9LPwJY4oMzOWROe8
AZS1ayRqlabG3qx3dPRcV4j6c6uROEfQY+HejCn+Zbi0CfVYtsI+xO5LkwnZMUyc
0qvy90h0PUQ2e37/wo5YnnVLK0ce1Gm6gY50iFXwqE69m55KHTufsIVX4eTUBfcj
PtugPtTnKEsW171r/gHPYO9A+WKxycCvjPs9Wogsljly+NrRzgPMAI7Alx//4lwq
QhwRWF1BkNOoCPEpHnVlLkcIhgdcAbbnvWxlcAVlLAQ/oYl6ShbQFv96y/2IWCVO
Mwr7Ab8a/dnyJ/GWEQdpJUmfKGQbKNpM93H5yD4AQlmz4Hj620gzm3y2XNJY2bUt
8H37Q2VWtlcRy2UHjgBSeF0YKCvdDuDmZwRZbPpajkmbcYSGtJFDve1/L6bOtZSj
7nVfYIqvtIBWrDZ3PF9G
=3uC5
-END PGP SIGNATURE-


0x92698E6A.asc
Description: application/pgp-keys


0x92698E6A.asc.sig
Description: PGP signature

Re: [PATCH 1/2] usb: gadget: f_midi: refactor state machine

[Felipe Ferreri Tonello]

Hi Clemens,

On 22/12/15 17:10, Clemens Ladisch wrote:
> Felipe F. Tonello wrote:
>> This refactor includes the following:
>>  * Cleaner state machine code;
> 
> It does not correctly handle system real time messages inserted between
> the status and data bytes of other messages.

True, thanks for pointing that out. I fixed that on next revision of
this patch.

> 
>>  * Reset state if MIDI message parsed is non-conformant;
> 
> Why?  In a byte stream like "C1 C3 44", where the data byte of the first
> message was lost, the reset would also drop the second message.

True. That was fixed as well.

> 
>>  * Fixed bug when a conformant MIDI message was followed by a non-conformant
>>causing the MIDI-USB message to use old temporary data (port->data[0..1]),
>>thus packing a wrong MIDI-USB request.
> 
> Running status is feature.

What do you mean by that? I don't qualify writing a *wrong* MIDI-USB
packet because of a previous MIDI message as a feature.

For instance, try this MIDI message:
"8A 54 24 00 40"

It will be converted to MIDI-USB as "08 8A 54 24 08 8A 00 40" which is
wrong. It should only be "08 8A 54 24" and ignore the "00 40" MIDI bytes.

On every state byte the message should basically reset data[0..1] to
zero overwriting previous data. This should also be true when a MIDI-USB
packet is complete.

Felipe


0x92698E6A.asc
Description: application/pgp-keys

Re: [PATCH 1/2] usb: gadget: f_midi: refactor state machine

[Clemens Ladisch]

Felipe Ferreri Tonello wrote:
>> Running status is feature.
>
>What do you mean by that?

That this behavior is intended, and required.

> I don't qualify writing a *wrong* MIDI-USB
>packet because of a previous MIDI message as a feature.

The MIDI Specification qualifies Running Status as a feature.


Regards,
Clemens

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 1/2] usb: gadget: f_midi: refactor state machine

[Felipe Ferreri Tonello]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA256

Hi Balbi,

On 22/12/15 17:49, Felipe Balbi wrote:
> 
> Hi,
> 
> "Felipe F. Tonello"  writes:
>> This refactor includes the following: * Cleaner state machine
>> code; * Reset state if MIDI message parsed is non-conformant; *
>> Fixed bug when a conformant MIDI message was followed by a
>> non-conformant causing the MIDI-USB message to use old temporary
>> data (port->data[0..1]), thus packing a wrong MIDI-USB request.
> 
> we don't do more than one logical thing per patch. Please split
> this up.

Actually this patch has only one logical change, the state machine
refactoring. But by doing it, those three items were a consequence.

Felipe
-BEGIN PGP SIGNATURE-
Version: GnuPG v2

iQIcBAEBCAAGBQJWemc+AAoJEMxEtNCSaY5qpXwP/1B/sVsfapRtP4dw93YF6En5
w/ej9JatIyJIaXNxauCVzgRl9uuiXGyEqErRjJdodyvHar3yKvD9HEXE6MhowEp4
JMD/phN2v9Sdj1VxRf9Z0XDSWsg6huVhfQU47HBMRGiY8ezvEgir2bvg7dYbZMsv
ACgIx8oh6N/AEHPq9GoLbEpXfJ58Pl564Sq/2o6wWsJQS06A+jp+JmqK4Y3eB5M5
18rmLW7lQLcZPO08Pf/c6BExWQLbzY/mT8KofwycvC9hWxYp9LPwJY4oMzOWROe8
AZS1ayRqlabG3qx3dPRcV4j6c6uROEfQY+HejCn+Zbi0CfVYtsI+xO5LkwnZMUyc
0qvy90h0PUQ2e37/wo5YnnVLK0ce1Gm6gY50iFXwqE69m55KHTufsIVX4eTUBfcj
PtugPtTnKEsW171r/gHPYO9A+WKxycCvjPs9Wogsljly+NrRzgPMAI7Alx//4lwq
QhwRWF1BkNOoCPEpHnVlLkcIhgdcAbbnvWxlcAVlLAQ/oYl6ShbQFv96y/2IWCVO
Mwr7Ab8a/dnyJ/GWEQdpJUmfKGQbKNpM93H5yD4AQlmz4Hj620gzm3y2XNJY2bUt
8H37Q2VWtlcRy2UHjgBSeF0YKCvdDuDmZwRZbPpajkmbcYSGtJFDve1/L6bOtZSj
7nVfYIqvtIBWrDZ3PF9G
=3uC5
-END PGP SIGNATURE-


0x92698E6A.asc
Description: application/pgp-keys


0x92698E6A.asc.sig
Description: PGP signature

Re: [PATCH 1/2] usb: gadget: f_midi: refactor state machine

[Felipe Balbi]

Hi,

"Felipe F. Tonello"  writes:
> This refactor includes the following:
>  * Cleaner state machine code;
>  * Reset state if MIDI message parsed is non-conformant;
>  * Fixed bug when a conformant MIDI message was followed by a non-conformant
>causing the MIDI-USB message to use old temporary data (port->data[0..1]),
>thus packing a wrong MIDI-USB request.

we don't do more than one logical thing per patch. Please split this up.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/2] usb: gadget: f_midi: refactor state machine

[Clemens Ladisch]

Felipe F. Tonello wrote:
> This refactor includes the following:
>  * Cleaner state machine code;

It does not correctly handle system real time messages inserted between
the status and data bytes of other messages.

>  * Reset state if MIDI message parsed is non-conformant;

Why?  In a byte stream like "C1 C3 44", where the data byte of the first
message was lost, the reset would also drop the second message.

>  * Fixed bug when a conformant MIDI message was followed by a non-conformant
>causing the MIDI-USB message to use old temporary data (port->data[0..1]),
>thus packing a wrong MIDI-USB request.

Running status is feature.


Regards,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH 1/2] usb: gadget: f_midi: refactor state machine

[Felipe F. Tonello]

This refactor includes the following:
 * Cleaner state machine code;
 * Reset state if MIDI message parsed is non-conformant;
 * Fixed bug when a conformant MIDI message was followed by a non-conformant
   causing the MIDI-USB message to use old temporary data (port->data[0..1]),
   thus packing a wrong MIDI-USB request.

Signed-off-by: Felipe F. Tonello 
---
 drivers/usb/gadget/function/f_midi.c | 243 ---
 1 file changed, 141 insertions(+), 102 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c 
b/drivers/usb/gadget/function/f_midi.c
index fb1fe96d..b70a830 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -50,6 +50,18 @@ static const char f_midi_longname[] = "MIDI Gadget";
  */
 #define MAX_PORTS 16
 
+/* MIDI message states */
+enum {
+   STATE_INITIAL = 0,
+   STATE_1PARAM,
+   STATE_2PARAM_1,
+   STATE_2PARAM_2,
+   STATE_SYSEX_0,
+   STATE_SYSEX_1,
+   STATE_SYSEX_2,
+   STATE_FINISHED,
+};
+
 /*
  * This is a gadget, and the IN/OUT naming is from the host's perspective.
  * USB -> OUT endpoint -> rawmidi
@@ -60,13 +72,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 +405,152 @@ 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;
-
-   if (b >= 0xf8) {
-   f_midi_transmit_packet(req, p0 | 0x0f, b, 0, 0);
-   } else if (b >= 0xf0) {
-   switch (b) {
-   case 0xf0:
-   port->data[0] = b;
-   port->state = STATE_SYSEX_1;
-   break;
-   case 0xf1:
-   case 0xf3:
-   port->data[0] = b;
-   port->state = STATE_1PARAM;
-   break;
-   case 0xf2:
+   uint8_t p[4] = { port->cable << 4, 0, 0, 0 };
+   uint8_t next_state = STATE_INITIAL;
+
+   switch (port->state) {
+   case STATE_INITIAL: {
+   if (b >= 0xf8) {
+   p[0] |= 0x0f;
+   p[1] = b;
+   next_state = STATE_FINISHED;
+   } else if (b >= 0xf0) {
+   switch (b) {
+   case 0xf0:
+   port->data[0] = b;
+   next_state = STATE_SYSEX_1;
+   break;
+   case 0xf1:
+   case 0xf3:
+   port->data[0] = b;
+   next_state = STATE_1PARAM;
+   break;
+   case 0xf2:
+   port->data[0] = b;
+   next_state = STATE_2PARAM_1;
+   break;
+   case 0xf4:
+   case 0xf5:
+   next_state = STATE_INITIAL;
+   break;
+   case 0xf6:
+   p[0] |= 0x05;
+   p[1] = 0xf6;
+   next_state = STATE_FINISHED;
+   break;
+   }
+   } else if (b >= 0x80) {
port->data[0] = b;
-   port->state = STATE_2PARAM_1;
-   break;
-   case 0xf4:
-   case 0xf5:
-   port->state = STATE_UNKNOWN;
-   break;
-   case 0xf6:
-   f_midi_transmit_packet(req, p0 | 0x05, 0xf6, 0, 0);
-   port->state = STATE_UNKNOWN;
-   break;
-   case 0xf7:
+   if (b >= 0xc0 && b <= 0xdf)
+   next_state = STATE_1PARAM;
+   else
+   next_state = STATE_2PARAM_1;
+   }
+   break;
+   }
+   case STATE_1PARAM:
+   case STATE_2PARAM_1:
+   case STATE_2PARAM_2:
+   case STATE_SYSEX_0:
+   case STATE_SYSEX_1:
+   

[PATCH 1/2] usb: gadget: f_midi: refactor state machine

[Felipe F. Tonello]

This refactor includes the following:
 * Cleaner state machine code;
 * Reset state if MIDI message parsed is non-conformant;
 * Fixed bug when a conformant MIDI message was followed by a non-conformant
   causing the MIDI-USB message to use old temporary data (port->data[0..1]),
   thus packing a wrong MIDI-USB request.

Signed-off-by: Felipe F. Tonello 
---
 drivers/usb/gadget/function/f_midi.c | 243 ---
 1 file changed, 141 insertions(+), 102 deletions(-)

diff --git a/drivers/usb/gadget/function/f_midi.c 
b/drivers/usb/gadget/function/f_midi.c
index fb1fe96d..b70a830 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -50,6 +50,18 @@ static const char f_midi_longname[] = "MIDI Gadget";
  */
 #define MAX_PORTS 16
 
+/* MIDI message states */
+enum {
+   STATE_INITIAL = 0,
+   STATE_1PARAM,
+   STATE_2PARAM_1,
+   STATE_2PARAM_2,
+   STATE_SYSEX_0,
+   STATE_SYSEX_1,
+   STATE_SYSEX_2,
+   STATE_FINISHED,
+};
+
 /*
  * This is a gadget, and the IN/OUT naming is from the host's perspective.
  * USB -> OUT endpoint -> rawmidi
@@ -60,13 +72,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 +405,152 @@ 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;
-
-   if (b >= 0xf8) {
-   f_midi_transmit_packet(req, p0 | 0x0f, b, 0, 0);
-   } else if (b >= 0xf0) {
-   switch (b) {
-   case 0xf0:
-   port->data[0] = b;
-   port->state = STATE_SYSEX_1;
-   break;
-   case 0xf1:
-   case 0xf3:
-   port->data[0] = b;
-   port->state = STATE_1PARAM;
-   break;
-   case 0xf2:
+   uint8_t p[4] = { port->cable << 4, 0, 0, 0 };
+   uint8_t next_state = STATE_INITIAL;
+
+   switch (port->state) {
+   case STATE_INITIAL: {
+   if (b >= 0xf8) {
+   p[0] |= 0x0f;
+   p[1] = b;
+   next_state = STATE_FINISHED;
+   } else if (b >= 0xf0) {
+   switch (b) {
+   case 0xf0:
+   port->data[0] = b;
+   next_state = STATE_SYSEX_1;
+   break;
+   case 0xf1:
+   case 0xf3:
+   port->data[0] = b;
+   next_state = STATE_1PARAM;
+   break;
+   case 0xf2:
+   port->data[0] = b;
+   next_state = STATE_2PARAM_1;
+   break;
+   case 0xf4:
+   case 0xf5:
+   next_state = STATE_INITIAL;
+   break;
+   case 0xf6:
+   p[0] |= 0x05;
+   p[1] = 0xf6;
+   next_state = STATE_FINISHED;
+   break;
+   }
+   } else if (b >= 0x80) {
port->data[0] = b;
-   port->state = STATE_2PARAM_1;
-   break;
-   case 0xf4:
-   case 0xf5:
-   port->state = STATE_UNKNOWN;
-   break;
-   case 0xf6:
-   f_midi_transmit_packet(req, p0 | 0x05, 0xf6, 0, 0);
-   port->state = STATE_UNKNOWN;
-   break;
-   case 0xf7:
+   if (b >= 0xc0 && b <= 0xdf)
+   next_state = STATE_1PARAM;
+   else
+   next_state = STATE_2PARAM_1;
+   }
+   break;
+   }
+   case STATE_1PARAM:
+   case STATE_2PARAM_1:
+   case STATE_2PARAM_2:
+   case STATE_SYSEX_0:
+   

Re: [PATCH 1/2] usb: gadget: f_midi: refactor state machine

[Felipe Balbi]

Hi,

"Felipe F. Tonello"  writes:
> This refactor includes the following:
>  * Cleaner state machine code;
>  * Reset state if MIDI message parsed is non-conformant;
>  * Fixed bug when a conformant MIDI message was followed by a non-conformant
>causing the MIDI-USB message to use old temporary data (port->data[0..1]),
>thus packing a wrong MIDI-USB request.

we don't do more than one logical thing per patch. Please split this up.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/2] usb: gadget: f_midi: refactor state machine

[Clemens Ladisch]

Felipe F. Tonello wrote:
> This refactor includes the following:
>  * Cleaner state machine code;

It does not correctly handle system real time messages inserted between
the status and data bytes of other messages.

>  * Reset state if MIDI message parsed is non-conformant;

Why?  In a byte stream like "C1 C3 44", where the data byte of the first
message was lost, the reset would also drop the second message.

>  * Fixed bug when a conformant MIDI message was followed by a non-conformant
>causing the MIDI-USB message to use old temporary data (port->data[0..1]),
>thus packing a wrong MIDI-USB request.

Running status is feature.


Regards,
Clemens
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/