Hi Balbi, On 09/03/16 07:04, Felipe Balbi wrote: > > Hi, > > "Felipe F. Tonello" <[email protected]> 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 <[email protected]> > > 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
