Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754830AbbKMMie (ORCPT ); Fri, 13 Nov 2015 07:38:34 -0500 Received: from mailout4.w1.samsung.com ([210.118.77.14]:36993 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754160AbbKMMia (ORCPT ); Fri, 13 Nov 2015 07:38:30 -0500 X-AuditID: cbfec7f4-f79c56d0000012ee-41-5645d9c242a3 Subject: Re: [PATCH v5 7/7] usb: gadget: f_midi: pre-allocate IN requests To: "Felipe F. Tonello" , linux-usb@vger.kernel.org References: <1447177929-22252-1-git-send-email-eu@felipetonello.com> <1447177929-22252-8-git-send-email-eu@felipetonello.com> Cc: linux-kernel@vger.kernel.org, Felipe Balbi , Greg Kroah-Hartman , Clemens Ladisch From: Robert Baldyga Message-id: <5645D9C1.30006@samsung.com> Date: Fri, 13 Nov 2015 13:38:25 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-version: 1.0 In-reply-to: <1447177929-22252-8-git-send-email-eu@felipetonello.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrELMWRmVeSWpSXmKPExsVy+t/xa7qHbrqGGSzfK21x8H69xZITD5ks OuefZbdoXryezeLyrjlsFouWtTI7sHnc/fqQ3ePHin5mj/1z17B7HL+xncnj8ya5ANYoLpuU 1JzMstQifbsEroy+ZfEFN/wrJrQ/Zm1gPGHbxcjJISFgIrHm20Q2CFtM4sK99WC2kMBSRokd 86q6GLmA7GeMEjfmv2EGSQgLeErMWT8LzBYBsk98XgdkcwAV1Ut0PtIAqWcWmMEo0bfjPtgg NgEdiS3fJzCC2LwCGhLL754Gs1kEVCUO7lgJZosKREhMnNDAClEjKPFj8j0WEJtTwFVi4c1X LCDzmQX0JO5f1AIJMwvIS2xe85Z5AqPALCQdsxCqZiGpWsDIvIpRNLU0uaA4KT3XUK84Mbe4 NC9dLzk/dxMjJKS/7GBcfMzqEKMAB6MSD2/SM5cwIdbEsuLK3EOMEhzMSiK8IYddw4R4UxIr q1KL8uOLSnNSiw8xSnOwKInzzt31PkRIID2xJDU7NbUgtQgmy8TBKdXA2PZ996uLdzjr5uq7 HJk7VWf+dXZtTZ3ezU1CC3n+bGR9/yrSYc4H32/na7MuCr9/17vv9jaWnG3nn3jVfr99xu73 OmueM1Wbjz1iPf/qMpOIrn/OuvwauY3Lo9OTzjR0Z8deZ2t8dlaUWTZp4+HTugVTjM7ZKuse PHlk/5yC1SYCRSk7nu50DVFiKc5INNRiLipOBADM5ko/ZQIAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10127 Lines: 323 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 > --- > 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 > #include > #include > +#include > > #include > #include > @@ -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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/