| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <5666D30D.8050703@felipetonello.com>
Date: Tue, 8 Dec 2015 12:54:37 +0000
From: Felipe Ferreri Tonello <eu@...ipetonello.com>
To: linux-usb@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Felipe Balbi <balbi@...com>,
Clemens Ladisch <clemens@...isch.de>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Robert Baldyga <r.baldyga@...sung.com>
Subject: Re: [PATCH v6 3/3] usb: gadget: f_midi: pre-allocate IN requests
On 01/12/15 18:31, 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@...ipetonello.com>
> ---
> drivers/usb/gadget/function/f_midi.c | 166 +++++++++++++++++++++++++++--------
> drivers/usb/gadget/legacy/gmidi.c | 2 +-
> 2 files changed, 129 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index 79dc611..fb1fe96d 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;
> + /* This fifo is used as a buffer ring for pre-allocated IN usb_requests */
> + DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
> + 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,20 @@ 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;
> +
> + req->length = 0;
> + req->complete = f_midi_complete;
> +
> + kfifo_put(&midi->in_req_fifo, req);
> + }
> +
> /* 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 =
> @@ -358,6 +379,7 @@ static void f_midi_disable(struct usb_function *f)
> {
> struct f_midi *midi = func_to_midi(f);
> struct usb_composite_dev *cdev = f->config->cdev;
> + struct usb_request *req = NULL;
>
> DBG(cdev, "disable\n");
>
> @@ -367,6 +389,10 @@ 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_get(&midi->in_req_fifo, &req))
> + free_ep_req(midi->in_ep, req);
> }
>
> static int f_midi_snd_free(struct snd_device *device)
> @@ -488,57 +514,113 @@ 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 buffer overrun, then we ignore this transmission.
> + * IMPORTANT: This will cause the user-space rawmidi device to block until a) usb
> + * requests have been completed or b) snd_rawmidi_write() times out. */
> + if (req->length > 0)
> + return;
> +
> + 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);
> + }
> + }
> + } while (active);
> +
> + return;
> +
> +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)
> @@ -664,6 +746,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 |
> @@ -1053,6 +1136,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);
> @@ -1126,6 +1210,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_last_port = 0;
> +
> + status = kfifo_alloc(&midi->in_req_fifo, midi->qlen, GFP_KERNEL);
> + if (status)
> + 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);
>
Just as a PS, I do have another patch refactoring the state machine on
this driver, but it requires this change to be applied. So once it gets
approved I will send the patch.
Felipe
Download attachment "0x92698E6A.asc" of type "application/pgp-keys" (7196 bytes)
Powered by blists - more mailing lists