Hi Balbi, On 09/03/16 07:04, Felipe Balbi wrote: > > Hi, > > "Felipe F. Tonello" <e...@felipetonello.com> writes: >> [ text/plain ] >> This refactor results in a cleaner state machine code and promotes >> consistency, readability, and maintanability of this driver. >> >> Signed-off-by: Felipe F. Tonello <e...@felipetonello.com> > > while working on this driver ... > >> --- >> 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 84c0ee5ebd1e..3cdb0741f3f8 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) > > ... could you get rid of the userspace types (as a separate patch, of > course) ?
userspace types? > >> { >> - uint8_t p0 = port->cable << 4; >> + uint8_t p[4] = { port->cable << 4, 0, 0, 0 }; >> + uint8_t next_state = STATE_INITIAL; > > and here too. I know you're going for consistency, but maybe it makes > sense to clean up the types before you cleanup the state machine. What do you mean by types? Do you mean to convert the state macros to an enum? > > [...] > >> + /* 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; > > okay, so this function will be called from ->complete() which is called > without controller lock held but with IRQs disabled. I wonder if you're > racing port->state here with this change. Could race on SMP, no ? Yes it will race because this function is not thread-safe. BTW, it would race anyway even without my patch. That's why we need that spin lock patch. It goes like this: USB request complete() or ALSA transmit(): f_midi_transmit() spin lock with irq call this refactored function() spin unlock with irq return return -- Felipe
0x92698E6A.asc
Description: application/pgp-keys