Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1266680 > unrolled thread
| Started by | "Felipe F. Tonello" <eu@felipetonello.com> |
|---|---|
| First post | 2015-11-10 19:00 +0100 |
| Last post | 2015-11-13 13:40 +0100 |
| Articles | 4 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH v5 0/7] USB MIDI Gadget improvements and bug fixes "Felipe F. Tonello" <eu@felipetonello.com> - 2015-11-10 19:00 +0100
[PATCH v5 7/7] usb: gadget: f_midi: pre-allocate IN requests "Felipe F. Tonello" <eu@felipetonello.com> - 2015-11-10 19:00 +0100
Re: [PATCH v5 7/7] usb: gadget: f_midi: pre-allocate IN requests Clemens Ladisch <clemens@ladisch.de> - 2015-11-13 10:00 +0100
Re: [PATCH v5 7/7] usb: gadget: f_midi: pre-allocate IN requests Robert Baldyga <r.baldyga@samsung.com> - 2015-11-13 13:40 +0100
| From | "Felipe F. Tonello" <eu@felipetonello.com> |
|---|---|
| Date | 2015-11-10 19:00 +0100 |
| Subject | [PATCH v5 0/7] USB MIDI Gadget improvements and bug fixes |
| Message-ID | <qtln3-8aF-7@gated-at.bofh.it> |
Patch 7 has changes on how to transmit IN USB requests. It implements a FIFO of pre-allocated usb requests and uses then as needed, instead of allocating then on demand. This is my initial implementation and is open for suggestions and comments. Patches 1-6 is pretty much straight forward. changes in v5: - Use ep->enabled insetad of creating driver specific flag - Save MIDIStreaming interface id in driver data - define free_ep_req as static inline in header changes in v4: - pre-alocation of in requests. - more code clean up - fix memory leak on out requests - configure endpoints only when setting up MIDIStreaming interface Felipe F. Tonello (7): usb: gadget: f_midi: Transmit data only when IN ep is enabled usb: gadget: f_midi: remove duplicated code usb: gadget: define free_ep_req as universal function usb: gadget: f_midi: fix leak on failed to enqueue out requests usb: gadget: gmidi: Cleanup legacy code usb: gadget: f_midi: set altsettings only for MIDIStreaming interface usb: gadget: f_midi: pre-allocate IN requests drivers/usb/gadget/function/f_midi.c | 196 ++++++++++++++++++++--------- drivers/usb/gadget/function/f_sourcesink.c | 6 - drivers/usb/gadget/function/g_zero.h | 1 - drivers/usb/gadget/legacy/gmidi.c | 10 +- drivers/usb/gadget/u_f.c | 1 - drivers/usb/gadget/u_f.h | 10 +- 6 files changed, 145 insertions(+), 79 deletions(-) -- 2.5.0 -- 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/
[toc] | [next] | [standalone]
| From | "Felipe F. Tonello" <eu@felipetonello.com> |
|---|---|
| Date | 2015-11-10 19:00 +0100 |
| Subject | [PATCH v5 7/7] usb: gadget: f_midi: pre-allocate IN requests |
| Message-ID | <qtln5-8aF-29@gated-at.bofh.it> |
| In reply to | #1266680 |
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@felipetonello.com>
---
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;
+
+ 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);
+ }
+ }
+ } 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);
--
2.5.0
--
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/
[toc] | [prev] | [next] | [standalone]
| From | Clemens Ladisch <clemens@ladisch.de> |
|---|---|
| Date | 2015-11-13 10:00 +0100 |
| Subject | Re: [PATCH v5 7/7] usb: gadget: f_midi: pre-allocate IN requests |
| Message-ID | <quin8-49R-5@gated-at.bofh.it> |
| In reply to | #1266681 |
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.
> ...
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -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;
As far as I can see, in_req_num must always have the same value as qlen.
> @@ -366,6 +388,20 @@ static void f_midi_disable(struct usb_function *f)
> + while (!kfifo_is_empty(&midi->in_req_fifo)) {
> + ...
> + 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);
> + }
kfifo_get() already checks for the FIFO being empty, so you can just
drop kfifi_is_empty().
> @@ -487,57 +523,111 @@ static void f_midi_transmit_byte(struct usb_request *req,
> ...
> +static void f_midi_transmit(struct f_midi *midi)
> +{
> +...
> + len = kfifo_peek(&midi->in_req_fifo, &req);
> + ...
> + 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;
> + }
There are two cases where the in_req FIFO might overflow:
1) the gadget is trying to send too much data at once; or
2) the host does not bother to read any of the data.
In case 1), the appropriate action would be to do nothing, so that the
remaining data is sent after some currently queued packets have been
transmitted. In case 2), the appropriate action would be to drop the
data (even better, the _oldest_ data), and spamming the log with error
messages would not help.
This code shows the error message for case 1), but does the action for
case 2).
I'm not quite sure if trying to detect which of these cases we have is
possible, or worthwhile. Anyway, with a packet size of 64, the queue
size would be 32*64 = 2KB, which should be enough for everyone. So I
propose to ignore case 1), and to drop the error message.
> @@ -1130,6 +1222,12 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
> + if (kfifo_alloc(&midi->in_req_fifo, midi->qlen, GFP_KERNEL))
> + goto setup_fail;
The setup_fail code expects an error code in the status variable.
Regards,
Clemens
--
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/
[toc] | [prev] | [next] | [standalone]
| From | Robert Baldyga <r.baldyga@samsung.com> |
|---|---|
| Date | 2015-11-13 13:40 +0100 |
| Subject | Re: [PATCH v5 7/7] usb: gadget: f_midi: pre-allocate IN requests |
| Message-ID | <qulO3-6pf-39@gated-at.bofh.it> |
| In reply to | #1266681 |
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@felipetonello.com>
> ---
> 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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web