Hi Balbi,
On 09/03/16 07:04, Felipe Balbi wrote:
>
> Hi,
>
> "Felipe F. Tonello" <eu@xxxxxxxxxxxxxxxxx> 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 <eu@xxxxxxxxxxxxxxxxx>
>
> 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
Attachment:
0x92698E6A.asc
Description: application/pgp-keys
