Re: [PATCH] usb: gadget: f_midi: refactor state machine

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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) ?

>  {
> -	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.

[...]

> +	/* 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 ?

-- 
balbi

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux