Re: [PATCH v5 7/7] usb: gadget: f_midi: pre-allocate IN requests

[Robert Baldyga]

Hi Felipe,

On 11/10/2015 06:52 PM, Felipe F. Tonello wrote:
> This patch introduces pre-allocation of IN endpoint USB requests. This
> improves on latency (requires no usb request allocation on transmit) and avoid
> several potential probles on allocating too many usb requests (which involves
> DMA pool allocation problems).
> 
> This implementation also handles better multiple MIDI Gadget ports, always
> processing the last processed MIDI substream if the last USB request wasn't
> enought to handle the whole stream.
> 
> Signed-off-by: Felipe F. Tonello <eu@xxxxxxxxxxxxxxxxx>
> ---
>  drivers/usb/gadget/function/f_midi.c | 174 +++++++++++++++++++++++++++--------
>  drivers/usb/gadget/legacy/gmidi.c    |   2 +-
>  2 files changed, 137 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index 615d632..6ac39f6 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -23,6 +23,7 @@
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/device.h>
> +#include <linux/kfifo.h>
>  
>  #include <sound/core.h>
>  #include <sound/initval.h>
> @@ -88,6 +89,9 @@ struct f_midi {
>  	int index;
>  	char *id;
>  	unsigned int buflen, qlen;
> +	DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
> +	unsigned int in_req_num;
> +	unsigned int in_last_port;
>  };
>  
>  static inline struct f_midi *func_to_midi(struct usb_function *f)
> @@ -95,7 +99,7 @@ static inline struct f_midi *func_to_midi(struct usb_function *f)
>  	return container_of(f, struct f_midi, func);
>  }
>  
> -static void f_midi_transmit(struct f_midi *midi, struct usb_request *req);
> +static void f_midi_transmit(struct f_midi *midi);
>  
>  DECLARE_UAC_AC_HEADER_DESCRIPTOR(1);
>  DECLARE_USB_MIDI_OUT_JACK_DESCRIPTOR(1);
> @@ -253,7 +257,8 @@ f_midi_complete(struct usb_ep *ep, struct usb_request *req)
>  		} else if (ep == midi->in_ep) {
>  			/* Our transmit completed. See if there's more to go.
>  			 * f_midi_transmit eats req, don't queue it again. */
> -			f_midi_transmit(midi, req);
> +			req->length = 0;
> +			f_midi_transmit(midi);
>  			return;
>  		}
>  		break;
> @@ -264,10 +269,12 @@ f_midi_complete(struct usb_ep *ep, struct usb_request *req)
>  	case -ESHUTDOWN:	/* disconnect from host */
>  		VDBG(cdev, "%s gone (%d), %d/%d\n", ep->name, status,
>  				req->actual, req->length);
> -		if (ep == midi->out_ep)
> +		if (ep == midi->out_ep) {
>  			f_midi_handle_out_data(ep, req);
> -
> -		free_ep_req(ep, req);
> +			/* We don't need to free IN requests because it's handled
> +			 * by the midi->in_req_fifo. */
> +			free_ep_req(ep, req);
> +		}
>  		return;
>  
>  	case -EOVERFLOW:	/* buffer overrun on read means that
> @@ -334,6 +341,21 @@ static int f_midi_set_alt(struct usb_function *f, unsigned intf, unsigned alt)
>  	if (err)
>  		return err;
>  
> +	/* pre-allocate write usb requests to use on f_midi_transmit. */
> +	while (kfifo_avail(&midi->in_req_fifo)) {
> +		struct usb_request *req =
> +			midi_alloc_ep_req(midi->in_ep, midi->buflen);
> +
> +		if (req == NULL)
> +			return -ENOMEM;

We need to free previously allocated requests in case of failure.

> +
> +		req->length = 0;
> +		req->complete = f_midi_complete;
> +
> +		kfifo_put(&midi->in_req_fifo, req);
> +		midi->in_req_num++;
> +	}
> +
>  	/* allocate a bunch of read buffers and queue them all at once. */
>  	for (i = 0; i < midi->qlen && err == 0; i++) {
>  		struct usb_request *req =
> @@ -366,6 +388,20 @@ static void f_midi_disable(struct usb_function *f)
>  	 */
>  	usb_ep_disable(midi->in_ep);
>  	usb_ep_disable(midi->out_ep);
> +
> +	/* release IN requests */
> +	while (!kfifo_is_empty(&midi->in_req_fifo)) {
> +		struct usb_request *req = NULL;
> +		unsigned int len;
> +
> +		len = kfifo_get(&midi->in_req_fifo, &req);
> +		if (len == 1)
> +			free_ep_req(midi->in_ep, req);
> +		else
> +			ERROR(midi, "%s couldn't free usb request: something went wrong with kfifo\n",
> +			      midi->in_ep->name);
> +	}
> +	midi->in_req_num = 0;
>  }
>  
>  static int f_midi_snd_free(struct snd_device *device)
> @@ -487,57 +523,111 @@ static void f_midi_transmit_byte(struct usb_request *req,
>  	}
>  }
>  
> -static void f_midi_transmit(struct f_midi *midi, struct usb_request *req)
> +static void f_midi_drop_out_substreams(struct f_midi *midi)
>  {
> -	struct usb_ep *ep = midi->in_ep;
> -	int i;
> -
> -	if (!ep)
> -		return;
> -
> -	if (!req)
> -		req = midi_alloc_ep_req(ep, midi->buflen);
> -
> -	if (!req) {
> -		ERROR(midi, "%s: alloc_ep_request failed\n", __func__);
> -		return;
> -	}
> -	req->length = 0;
> -	req->complete = f_midi_complete;
> +	unsigned int i;
>  
>  	for (i = 0; i < MAX_PORTS; i++) {
>  		struct gmidi_in_port *port = midi->in_port[i];
>  		struct snd_rawmidi_substream *substream = midi->in_substream[i];
>  
> -		if (!port || !port->active || !substream)
> +		if (!port)
> +			break;
> +
> +		if (!port->active || !substream)
>  			continue;
>  
> -		while (req->length + 3 < midi->buflen) {
> -			uint8_t b;
> -			if (snd_rawmidi_transmit(substream, &b, 1) != 1) {
> -				port->active = 0;
> +		snd_rawmidi_drop_output(substream);
> +	}
> +}
> +
> +static void f_midi_transmit(struct f_midi *midi)
> +{
> +	struct usb_ep *ep = midi->in_ep;
> +	bool active;
> +
> +	/* We only care about USB requests if IN endpoint is enabled */
> +	if (!ep || !ep->enabled)
> +		goto drop_out;
> +
> +	do {
> +		struct usb_request *req = NULL;
> +		unsigned int len, i;
> +
> +		active = false;
> +
> +		/* We peek the request in order to reuse it if it fails
> +		 * to enqueue on its endpoint */
> +		len = kfifo_peek(&midi->in_req_fifo, &req);
> +		if (len != 1) {
> +			ERROR(midi, "%s: Couldn't get usb request\n", __func__);
> +			goto drop_out;
> +		}
> +
> +		if (req->length > 0) {
> +			WARNING(midi, "%s: All USB requests have been used. Current queue size "
> +				"is %u, consider increasing it.\n", __func__, midi->in_req_num);
> +			goto drop_out;
> +		}
> +
> +		for (i = midi->in_last_port; i < MAX_PORTS; i++) {
> +			struct gmidi_in_port *port = midi->in_port[i];
> +			struct snd_rawmidi_substream *substream = midi->in_substream[i];
> +
> +			if (!port) {
> +				/* Reset counter when we reach the last available port */
> +				midi->in_last_port = 0;
> +				break;
> +			}
> +
> +			if (!port->active || !substream)
> +				continue;
> +
> +			while (req->length + 3 < midi->buflen) {
> +				uint8_t b;
> +
> +				if (snd_rawmidi_transmit(substream, &b, 1) != 1) {
> +					port->active = 0;
> +					break;
> +				}
> +				f_midi_transmit_byte(req, port, b);
> +			}
> +
> +			active = !!port->active;
> +			/* Check if last port is still active, which means that
> +			 * there is still data on that substream but this current
> +			 * request run out of space. */
> +			if (active) {
> +				midi->in_last_port = i;
> +				/* There is no need to re-iterate though midi ports. */
>  				break;
>  			}
> -			f_midi_transmit_byte(req, port, b);
>  		}
> -	}
>  
> -	if (req->length > 0 && ep->enabled) {
> -		int err;
> +		if (req->length > 0) {
> +			int err;
>  
> -		err = usb_ep_queue(ep, req, GFP_ATOMIC);
> -		if (err < 0)
> -			ERROR(midi, "%s queue req: %d\n",
> -			      midi->in_ep->name, err);
> -	} else {
> -		free_ep_req(ep, req);
> -	}
> +			err = usb_ep_queue(ep, req, GFP_ATOMIC);
> +			if (err < 0) {
> +				ERROR(midi, "%s failed to queue req: %d\n",
> +				      midi->in_ep->name, err);
> +				req->length = 0; /* Re-use request next time. */
> +			} else {
> +				/* Upon success, put request at the back of the queue. */
> +				kfifo_skip(&midi->in_req_fifo);
> +				kfifo_put(&midi->in_req_fifo, req);

There are simpler and clearer ways to implement cyclic buffer than using
kfifo. To be honest, it took ma a while to realize what's going on. As
long as you mark unused requests by setting req->length to zero you only
need to store index of last used req to be able to achieve the same effect.

> +			}
> +		}
> +	} while (active);
> +
> +drop_out:
> +	f_midi_drop_out_substreams(midi);
>  }
>  
>  static void f_midi_in_tasklet(unsigned long data)
>  {
>  	struct f_midi *midi = (struct f_midi *) data;
> -	f_midi_transmit(midi, NULL);
> +	f_midi_transmit(midi);
>  }
>  
>  static int f_midi_in_open(struct snd_rawmidi_substream *substream)
> @@ -663,6 +753,7 @@ static int f_midi_register_card(struct f_midi *midi)
>  		goto fail;
>  	}
>  	midi->rmidi = rmidi;
> +	midi->in_last_port = 0;
>  	strcpy(rmidi->name, card->shortname);
>  	rmidi->info_flags = SNDRV_RAWMIDI_INFO_OUTPUT |
>  			    SNDRV_RAWMIDI_INFO_INPUT |
> @@ -1057,6 +1148,7 @@ static void f_midi_free(struct usb_function *f)
>  	mutex_lock(&opts->lock);
>  	for (i = opts->in_ports - 1; i >= 0; --i)
>  		kfree(midi->in_port[i]);
> +	kfifo_free(&midi->in_req_fifo);
>  	kfree(midi);
>  	--opts->refcnt;
>  	mutex_unlock(&opts->lock);
> @@ -1130,6 +1222,12 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
>  	midi->index = opts->index;
>  	midi->buflen = opts->buflen;
>  	midi->qlen = opts->qlen;
> +	midi->in_req_num = 0;
> +	midi->in_last_port = 0;
> +
> +	if (kfifo_alloc(&midi->in_req_fifo, midi->qlen, GFP_KERNEL))
> +		goto setup_fail;
> +
>  	++opts->refcnt;
>  	mutex_unlock(&opts->lock);
>  
> diff --git a/drivers/usb/gadget/legacy/gmidi.c b/drivers/usb/gadget/legacy/gmidi.c
> index be8e91d..f68c188 100644
> --- a/drivers/usb/gadget/legacy/gmidi.c
> +++ b/drivers/usb/gadget/legacy/gmidi.c
> @@ -53,7 +53,7 @@ MODULE_PARM_DESC(buflen, "MIDI buffer length");
>  
>  static unsigned int qlen = 32;
>  module_param(qlen, uint, S_IRUGO);
> -MODULE_PARM_DESC(qlen, "USB read request queue length");
> +MODULE_PARM_DESC(qlen, "USB read and write request queue length");
>  
>  static unsigned int in_ports = 1;
>  module_param(in_ports, uint, S_IRUGO);
> 

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