Patches are pretty much self-described.
Patch 1 is revised from comments.
Patch 2 is a bug fix.
Felipe F. Tonello (5):
usb: gadget: f_midi: refactor state machine
usb: gadget: f_midi: added spinlock on transmit function
usb: gadget: gmidi: remove bus powered requirement on bmAttributes
usb: gadget: f_midi: cleanups and typos fixes
usb: gadget: f_midi: updated copyright
drivers/usb/gadget/function/f_midi.c | 293 ++++++++++++++++++++++-------------
drivers/usb/gadget/legacy/gmidi.c | 14 +-
2 files changed, 193 insertions(+), 114 deletions(-)
--
2.7.2
Since f_midi_transmit is called by both ALSA and USB frameworks, it can
potentially cause a race condition between both calls. This is bad because the
way f_midi_transmit is implemented can't handle concurrent calls. This is due
to the fact that the usb request fifo looks for the next element and only if
it has data to process it enqueues the request, otherwise re-uses it. If both
(ALSA and USB) frameworks calls this function at the same time, the
kfifo_seek() will return the same usb_request, which will cause a race
condition.
To solve this problem a syncronization mechanism is necessary. In this case it
is used a spinlock since f_midi_transmit is also called by usb_request->complete
callback in interrupt context.
On benchmarks realized by me, spinlocks were more efficient then scheduling
the f_midi_transmit tasklet in process context and using a mutex to
synchronize. Also it performs better then previous implementation that
allocated a usb_request for every new transmit made.
Signed-off-by: Felipe F. Tonello <[email protected]>
---
drivers/usb/gadget/function/f_midi.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 3cdb0741f3f8..8475e3dc82d4 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -24,6 +24,7 @@
#include <linux/slab.h>
#include <linux/device.h>
#include <linux/kfifo.h>
+#include <linux/spinlock.h>
#include <sound/core.h>
#include <sound/initval.h>
@@ -95,6 +96,7 @@ struct f_midi {
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 *);
+ spinlock_t transmit_lock;
unsigned int in_last_port;
struct gmidi_in_port in_ports_array[/* in_ports */];
@@ -651,17 +653,22 @@ static void f_midi_transmit(struct f_midi *midi)
{
struct usb_ep *ep = midi->in_ep;
int ret;
+ unsigned long flags;
/* We only care about USB requests if IN endpoint is enabled */
if (!ep || !ep->enabled)
goto drop_out;
+ spin_lock_irqsave(&midi->transmit_lock, flags);
+
do {
ret = f_midi_do_transmit(midi, ep);
if (ret < 0)
goto drop_out;
} while (ret);
+ spin_unlock_irqrestore(&midi->transmit_lock, flags);
+
return;
drop_out:
@@ -1255,6 +1262,8 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
if (status)
goto setup_fail;
+ spin_lock_init(&midi->transmit_lock);
+
++opts->refcnt;
mutex_unlock(&opts->lock);
--
2.7.2
Signed-off-by: Felipe F. Tonello <[email protected]>
---
drivers/usb/gadget/function/f_midi.c | 77 +++++++++++++++++++-----------------
1 file changed, 40 insertions(+), 37 deletions(-)
diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 8475e3dc82d4..9a9e6112e224 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -1,5 +1,5 @@
/*
- * f_midi.c -- USB MIDI class function driver
+ * f_midi.c -- USB-MIDI class function driver
*
* Copyright (C) 2006 Thumtronics Pty Ltd.
* Developed for Thumtronics by Grey Innovation
@@ -16,7 +16,7 @@
* Copyright (C) 2006 Thumtronics Pty Ltd.
* Ben Williamson <[email protected]>
*
- * Licensed under the GPL-2 or later.
+ * Licensed under the GPLv2.
*/
#include <linux/kernel.h>
@@ -41,8 +41,8 @@
MODULE_AUTHOR("Ben Williamson");
MODULE_LICENSE("GPL v2");
-static const char f_midi_shortname[] = "f_midi";
-static const char f_midi_longname[] = "MIDI Gadget";
+static const char f_midi_shortname[] = "f_midi";
+static const char f_midi_longname[] = "MIDI Gadget";
/*
* We can only handle 16 cables on one single endpoint, as cable numbers are
@@ -78,28 +78,31 @@ struct gmidi_in_port {
};
struct f_midi {
- struct usb_function func;
- struct usb_gadget *gadget;
- struct usb_ep *in_ep, *out_ep;
- struct snd_card *card;
- struct snd_rawmidi *rmidi;
- u8 ms_id;
-
- struct snd_rawmidi_substream *out_substream[MAX_PORTS];
-
- unsigned long out_triggered;
- struct tasklet_struct tasklet;
+ struct usb_function func;
+ struct usb_gadget *gadget;
+ struct usb_ep *in_ep, *out_ep;
+ u8 ms_id;
+ unsigned long out_triggered;
unsigned int in_ports;
unsigned int out_ports;
- int index;
- char *id;
- unsigned int buflen, qlen;
+ unsigned int buflen;
+ unsigned int qlen;
+ unsigned int len;
+
/* This fifo is used as a buffer ring for pre-allocated IN usb_requests */
DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
spinlock_t transmit_lock;
+
+ /* ALSA stuff */
+ struct snd_card *card;
+ struct snd_rawmidi *rmidi;
+ struct snd_rawmidi_substream *out_substream[MAX_PORTS];
+ struct tasklet_struct tasklet;
unsigned int in_last_port;
+ int index;
+ char *id;
- struct gmidi_in_port in_ports_array[/* in_ports */];
+ struct gmidi_in_port in_ports_array[/* in_ports */];
};
static inline struct f_midi *func_to_midi(struct usb_function *f)
@@ -191,7 +194,7 @@ static struct usb_ms_endpoint_descriptor_16 ms_in_desc = {
/* string IDs are assigned dynamically */
-#define STRING_FUNC_IDX 0
+#define STRING_FUNC_IDX 0
static struct usb_string midi_string_defs[] = {
[STRING_FUNC_IDX].s = "MIDI function",
@@ -199,7 +202,7 @@ static struct usb_string midi_string_defs[] = {
};
static struct usb_gadget_strings midi_stringtab = {
- .language = 0x0409, /* en-us */
+ .language = 0x0409, /* en-us */
.strings = midi_string_defs,
};
@@ -409,7 +412,7 @@ static int f_midi_snd_free(struct snd_device *device)
}
/*
- * Converts MIDI commands to USB MIDI packets.
+ * 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)
@@ -956,15 +959,15 @@ static int f_midi_bind(struct usb_configuration *c, struct usb_function *f)
in_emb->iJack = 0;
midi_function[i++] = (struct usb_descriptor_header *) in_emb;
- out_ext->bLength = USB_DT_MIDI_OUT_SIZE(1);
- out_ext->bDescriptorType = USB_DT_CS_INTERFACE;
- out_ext->bDescriptorSubtype = USB_MS_MIDI_OUT_JACK;
- out_ext->bJackType = USB_MS_EXTERNAL;
- out_ext->bJackID = jack++;
- out_ext->bNrInputPins = 1;
- out_ext->iJack = 0;
- out_ext->pins[0].baSourceID = in_emb->bJackID;
- out_ext->pins[0].baSourcePin = 1;
+ out_ext->bLength = USB_DT_MIDI_OUT_SIZE(1);
+ out_ext->bDescriptorType = USB_DT_CS_INTERFACE;
+ out_ext->bDescriptorSubtype = USB_MS_MIDI_OUT_JACK;
+ out_ext->bJackType = USB_MS_EXTERNAL;
+ out_ext->bJackID = jack++;
+ out_ext->bNrInputPins = 1;
+ out_ext->iJack = 0;
+ out_ext->pins[0].baSourceID = in_emb->bJackID;
+ out_ext->pins[0].baSourcePin = 1;
midi_function[i++] = (struct usb_descriptor_header *) out_ext;
/* link it to the endpoint */
@@ -1251,12 +1254,12 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
status = -ENOMEM;
goto setup_fail;
}
- midi->in_ports = opts->in_ports;
- midi->out_ports = opts->out_ports;
- midi->index = opts->index;
- midi->buflen = opts->buflen;
- midi->qlen = opts->qlen;
- midi->in_last_port = 0;
+ midi->in_ports = opts->in_ports;
+ midi->out_ports = opts->out_ports;
+ 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)
--
2.7.2
Signed-off-by: Felipe F. Tonello <[email protected]>
---
drivers/usb/gadget/function/f_midi.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
index 9a9e6112e224..5c7f5c780fda 100644
--- a/drivers/usb/gadget/function/f_midi.c
+++ b/drivers/usb/gadget/function/f_midi.c
@@ -5,6 +5,9 @@
* Developed for Thumtronics by Grey Innovation
* Ben Williamson <[email protected]>
*
+ * Copyright (C) 2015,2016 ROLI Ltd.
+ * Felipe F. Tonello <[email protected]>
+ *
* Rewritten for the composite framework
* Copyright (C) 2011 Daniel Mack <[email protected]>
*
--
2.7.2
This gadget uses a bmAttributes and MaxPower that requires the USB bus to be
powered from the host, which is not correct because this configuration is device
specific, not a USB-MIDI requirement.
This patch adds two modules parameters, bmAttributes and MaxPower, that allows
the user to set those flags. The default values are what the gadget used to use
for backward compatibility.
Signed-off-by: Felipe F. Tonello <[email protected]>
---
drivers/usb/gadget/legacy/gmidi.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/legacy/gmidi.c b/drivers/usb/gadget/legacy/gmidi.c
index fc2ac150f5ff..0553435cc360 100644
--- a/drivers/usb/gadget/legacy/gmidi.c
+++ b/drivers/usb/gadget/legacy/gmidi.c
@@ -63,6 +63,14 @@ static unsigned int out_ports = 1;
module_param(out_ports, uint, S_IRUGO);
MODULE_PARM_DESC(out_ports, "Number of MIDI output ports");
+static unsigned int bmAttributes = USB_CONFIG_ATT_ONE;
+module_param(bmAttributes, uint, S_IRUGO);
+MODULE_PARM_DESC(bmAttributes, "Configuration Descriptor's bmAttributes parameter");
+
+static unsigned int MaxPower = CONFIG_USB_GADGET_VBUS_DRAW;
+module_param(MaxPower, uint, S_IRUGO);
+MODULE_PARM_DESC(MaxPower, "Used to calculate Configuration Descriptor's bMaxPower parameter");
+
/* Thanks to Grey Innovation for donating this product ID.
*
* DO NOT REUSE THESE IDs with a protocol-incompatible driver!! Ever!!
@@ -119,8 +127,8 @@ static struct usb_configuration midi_config = {
.label = "MIDI Gadget",
.bConfigurationValue = 1,
/* .iConfiguration = DYNAMIC */
- .bmAttributes = USB_CONFIG_ATT_ONE,
- .MaxPower = CONFIG_USB_GADGET_VBUS_DRAW,
+ /* .bmAttributes = DYNAMIC */
+ /* .MaxPower = DYNAMIC */
};
static int midi_bind_config(struct usb_configuration *c)
@@ -163,6 +171,8 @@ static int midi_bind(struct usb_composite_dev *cdev)
device_desc.iManufacturer = strings_dev[USB_GADGET_MANUFACTURER_IDX].id;
device_desc.iProduct = strings_dev[USB_GADGET_PRODUCT_IDX].id;
midi_config.iConfiguration = strings_dev[STRING_DESCRIPTION_IDX].id;
+ midi_config.bmAttributes = bmAttributes;
+ midi_config.MaxPower = MaxPower;
status = usb_add_config(cdev, &midi_config, midi_bind_config);
if (status < 0)
--
2.7.2
This refactor results in a cleaner state machine code and as a result fixed a
bug when packaging a USB-MIDI packet right after a non-conformant MIDI byte stream.
Signed-off-by: Felipe F. Tonello <[email protected]>
---
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)
{
- uint8_t p0 = port->cable << 4;
+ uint8_t p[4] = { port->cable << 4, 0, 0, 0 };
+ uint8_t next_state = STATE_INITIAL;
+
+ switch (b) {
+ case 0xf8 ... 0xff:
+ /* System Real-Time Messages */
+ p[0] |= 0x0f;
+ p[1] = b;
+ next_state = port->state;
+ port->state = STATE_REAL_TIME;
+ break;
+
+ case 0xf7:
+ /* End of SysEx */
+ switch (port->state) {
+ case STATE_SYSEX_0:
+ p[0] |= 0x05;
+ p[1] = 0xf7;
+ next_state = STATE_FINISHED;
+ break;
+ case STATE_SYSEX_1:
+ p[0] |= 0x06;
+ p[1] = port->data[0];
+ p[2] = 0xf7;
+ next_state = STATE_FINISHED;
+ break;
+ case STATE_SYSEX_2:
+ p[0] |= 0x07;
+ p[1] = port->data[0];
+ p[2] = port->data[1];
+ p[3] = 0xf7;
+ next_state = STATE_FINISHED;
+ break;
+ default:
+ /* Ignore byte */
+ next_state = port->state;
+ port->state = STATE_INITIAL;
+ }
+ break;
- if (b >= 0xf8) {
- f_midi_transmit_packet(req, p0 | 0x0f, b, 0, 0);
- } else if (b >= 0xf0) {
+ case 0xf0 ... 0xf6:
+ /* System Common Messages */
+ port->data[0] = port->data[1] = 0;
+ port->state = STATE_INITIAL;
switch (b) {
case 0xf0:
port->data[0] = b;
- port->state = STATE_SYSEX_1;
+ port->data[1] = 0;
+ next_state = STATE_SYSEX_1;
break;
case 0xf1:
case 0xf3:
port->data[0] = b;
- port->state = STATE_1PARAM;
+ next_state = STATE_1PARAM;
break;
case 0xf2:
port->data[0] = b;
- port->state = STATE_2PARAM_1;
+ next_state = STATE_2PARAM_1;
break;
case 0xf4:
case 0xf5:
- port->state = STATE_UNKNOWN;
+ next_state = STATE_INITIAL;
break;
case 0xf6:
- f_midi_transmit_packet(req, p0 | 0x05, 0xf6, 0, 0);
- port->state = STATE_UNKNOWN;
- break;
- case 0xf7:
- switch (port->state) {
- case STATE_SYSEX_0:
- f_midi_transmit_packet(req,
- p0 | 0x05, 0xf7, 0, 0);
- break;
- case STATE_SYSEX_1:
- f_midi_transmit_packet(req,
- p0 | 0x06, port->data[0], 0xf7, 0);
- break;
- case STATE_SYSEX_2:
- f_midi_transmit_packet(req,
- p0 | 0x07, port->data[0],
- port->data[1], 0xf7);
- break;
- }
- port->state = STATE_UNKNOWN;
+ p[0] |= 0x05;
+ p[1] = 0xf6;
+ next_state = STATE_FINISHED;
break;
}
- } else if (b >= 0x80) {
+ break;
+
+ case 0x80 ... 0xef:
+ /*
+ * Channel Voice Messages, Channel Mode Messages
+ * and Control Change Messages.
+ */
port->data[0] = b;
+ port->data[1] = 0;
+ port->state = STATE_INITIAL;
if (b >= 0xc0 && b <= 0xdf)
- port->state = STATE_1PARAM;
+ next_state = STATE_1PARAM;
else
- port->state = STATE_2PARAM_1;
- } else { /* b < 0x80 */
+ next_state = STATE_2PARAM_1;
+ break;
+
+ case 0x00 ... 0x7f:
+ /* Message parameters */
switch (port->state) {
case STATE_1PARAM:
- if (port->data[0] < 0xf0) {
- p0 |= port->data[0] >> 4;
- } else {
- p0 |= 0x02;
- port->state = STATE_UNKNOWN;
- }
- f_midi_transmit_packet(req, p0, port->data[0], b, 0);
+ if (port->data[0] < 0xf0)
+ p[0] |= port->data[0] >> 4;
+ else
+ p[0] |= 0x02;
+
+ p[1] = port->data[0];
+ p[2] = b;
+ /* This is to allow Running State Messages */
+ next_state = STATE_1PARAM;
break;
case STATE_2PARAM_1:
port->data[1] = b;
- port->state = STATE_2PARAM_2;
+ next_state = STATE_2PARAM_2;
break;
case STATE_2PARAM_2:
- if (port->data[0] < 0xf0) {
- p0 |= port->data[0] >> 4;
- port->state = STATE_2PARAM_1;
- } else {
- p0 |= 0x03;
- port->state = STATE_UNKNOWN;
- }
- f_midi_transmit_packet(req,
- p0, port->data[0], port->data[1], b);
+ if (port->data[0] < 0xf0)
+ p[0] |= port->data[0] >> 4;
+ else
+ p[0] |= 0x03;
+
+ p[1] = port->data[0];
+ p[2] = port->data[1];
+ p[3] = b;
+ /* This is to allow Running State Messages */
+ next_state = STATE_2PARAM_1;
break;
case STATE_SYSEX_0:
port->data[0] = b;
- port->state = STATE_SYSEX_1;
+ next_state = STATE_SYSEX_1;
break;
case STATE_SYSEX_1:
port->data[1] = b;
- port->state = STATE_SYSEX_2;
+ next_state = STATE_SYSEX_2;
break;
case STATE_SYSEX_2:
- f_midi_transmit_packet(req,
- p0 | 0x04, port->data[0], port->data[1], b);
- port->state = STATE_SYSEX_0;
+ p[0] |= 0x04;
+ p[1] = port->data[0];
+ p[2] = port->data[1];
+ p[3] = b;
+ next_state = STATE_SYSEX_0;
break;
}
+ break;
+ }
+
+ /* 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;
}
static void f_midi_drop_out_substreams(struct f_midi *midi)
@@ -631,7 +685,7 @@ static int f_midi_in_open(struct snd_rawmidi_substream *substream)
VDBG(midi, "%s()\n", __func__);
port = midi->in_ports_array + substream->number;
port->substream = substream;
- port->state = STATE_UNKNOWN;
+ port->state = STATE_INITIAL;
return 0;
}
--
2.7.2
Felipe F. Tonello wrote:
> This refactor results in a cleaner state machine code
It increases the number of states, and now juggles two state variables.
I cannot agree to it being cleaner.
> and as a result fixed a bug when packaging a USB-MIDI packet right after
> a non-conformant MIDI byte stream.
I have been unable to determine where exactly the new code behaves
differently. Can you show an example?
Regards,
Clemens
Hi Clemens,
On 02/03/16 21:09, Clemens Ladisch wrote:
> Felipe F. Tonello wrote:
>> This refactor results in a cleaner state machine code
>
> It increases the number of states, and now juggles two state variables.
> I cannot agree to it being cleaner.
Yes, it increases the number of states. That was done in order to
actually implement a proper finite state machine with one state at a
time and a transition state. The result is a much cleaner MIDI parser
that is easy to maintain and read.
I recommend you to apply the patch yourself (it's on top of Balbi's next
branch) because the patch can be confusing to understand the end result.
>
>> and as a result fixed a bug when packaging a USB-MIDI packet right after
>> a non-conformant MIDI byte stream.
>
> I have been unable to determine where exactly the new code behaves
> differently. Can you show an example?
Sorry, I forgot to remove this comment since your last revision. There
is no bug I could reproduce with the previous parser.
--
Felipe
Felipe Ferreri Tonello wrote:
> On 02/03/16 21:09, Clemens Ladisch wrote:
>> Felipe F. Tonello wrote:
>>> This refactor results in a cleaner state machine code
>>
>> It increases the number of states, and now juggles two state variables.
>> I cannot agree to it being cleaner.
>
> Yes, it increases the number of states. That was done in order to
> actually implement a proper finite state machine with one state at a
> time and a transition state.
I know, "clean" is subjective. But in what way was the old state
machine not "proper"?
And how is handling two states (port->state and next_state) cleaner?
As far as I can tell, the requirement for a separate variable comes not
from any inherent complexity of the state machine itself, but only
because the transmit_packet function was inlined.
Regards,
Clemens
Hi Clemens,
On 03/03/16 11:38, Clemens Ladisch wrote:
> Felipe Ferreri Tonello wrote:
>> On 02/03/16 21:09, Clemens Ladisch wrote:
>>> Felipe F. Tonello wrote:
>>>> This refactor results in a cleaner state machine code
>>>
>>> It increases the number of states, and now juggles two state variables.
>>> I cannot agree to it being cleaner.
>>
>> Yes, it increases the number of states. That was done in order to
>> actually implement a proper finite state machine with one state at a
>> time and a transition state.
>
> I know, "clean" is subjective.
Clean is subjective, yes. However, based on our common sense and
experience we can discern on what is clean and what is not. There is
also good literature about the subject that we can always consider.
> But in what way was the old state
> machine not "proper"?
Because it didn't reflect all the correct and possible MIDI states and
there was no transitional state.
>
> And how is handling two states (port->state and next_state) cleaner?
> As far as I can tell, the requirement for a separate variable comes not
> from any inherent complexity of the state machine itself, but only
> because the transmit_packet function was inlined.
next_state is a transitional state, thus the temporal nature.
This patch doesn't change any functionality. But the important thing
here is that it improves the driver maintainability by making the state
machine cleaner (which is one of the most important pieces of code of
the driver). I call it clean because on each circumstance of each state
it's clear on what is about to happen to the USB request and to the
port's buffers.
I confess I would not spend the time on it just for puritanisms, but I
found myself a hard time while debugging it.
--
Felipe
Hi,
"Felipe F. Tonello" <[email protected]> writes:
> [ text/plain ]
> Patches are pretty much self-described.
>
> Patch 1 is revised from comments.
you really need to describe what you changed. This also should have v2
on subject line.
I guess it's too late to get this in v4.6 merge window as I'm already
applying the last few patches and plan to send a pull request in a few
minutes.
--
balbi
"Felipe F. Tonello" <[email protected]> writes:
> [ text/plain ]
> Signed-off-by: Felipe F. Tonello <[email protected]>
no commit log == no commit
> ---
> drivers/usb/gadget/function/f_midi.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index 9a9e6112e224..5c7f5c780fda 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -5,6 +5,9 @@
> * Developed for Thumtronics by Grey Innovation
> * Ben Williamson <[email protected]>
> *
> + * Copyright (C) 2015,2016 ROLI Ltd.
> + * Felipe F. Tonello <[email protected]>
Did you check with your company's lawyer that your changes are enough to
justify a copyright ?
--
balbi
"Felipe F. Tonello" <[email protected]> writes:
> [ text/plain ]
> Signed-off-by: Felipe F. Tonello <[email protected]>
no commit log == no commit
--
balbi
Hi,
"Felipe F. Tonello" <[email protected]> writes:
> [ text/plain ]
> This gadget uses a bmAttributes and MaxPower that requires the USB bus to be
> powered from the host, which is not correct because this configuration is device
> specific, not a USB-MIDI requirement.
>
> This patch adds two modules parameters, bmAttributes and MaxPower, that allows
> the user to set those flags. The default values are what the gadget used to use
> for backward compatibility.
>
> Signed-off-by: Felipe F. Tonello <[email protected]>
> ---
> drivers/usb/gadget/legacy/gmidi.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/legacy/gmidi.c b/drivers/usb/gadget/legacy/gmidi.c
> index fc2ac150f5ff..0553435cc360 100644
> --- a/drivers/usb/gadget/legacy/gmidi.c
> +++ b/drivers/usb/gadget/legacy/gmidi.c
> @@ -63,6 +63,14 @@ static unsigned int out_ports = 1;
> module_param(out_ports, uint, S_IRUGO);
> MODULE_PARM_DESC(out_ports, "Number of MIDI output ports");
>
> +static unsigned int bmAttributes = USB_CONFIG_ATT_ONE;
> +module_param(bmAttributes, uint, S_IRUGO);
> +MODULE_PARM_DESC(bmAttributes, "Configuration Descriptor's bmAttributes parameter");
> +
> +static unsigned int MaxPower = CONFIG_USB_GADGET_VBUS_DRAW;
> +module_param(MaxPower, uint, S_IRUGO);
> +MODULE_PARM_DESC(MaxPower, "Used to calculate Configuration Descriptor's bMaxPower parameter");
you didn't run checkpatch, did you ? Also, are you sure you will need to
change this by simply reloading the module ? I'm not convinced.
> @@ -119,8 +127,8 @@ static struct usb_configuration midi_config = {
> .label = "MIDI Gadget",
> .bConfigurationValue = 1,
> /* .iConfiguration = DYNAMIC */
> - .bmAttributes = USB_CONFIG_ATT_ONE,
nack, nackety nack nack nack :-)
USB_CONFIG_ATT_ONE *must* always be set. With your module parameter you
give users the oportunity to violate USB spec.
--
balbi
Hi,
"Felipe F. Tonello" <[email protected]> writes:
> [ text/plain ]
> Since f_midi_transmit is called by both ALSA and USB frameworks, it can
> potentially cause a race condition between both calls. This is bad because the
> way f_midi_transmit is implemented can't handle concurrent calls. This is due
> to the fact that the usb request fifo looks for the next element and only if
> it has data to process it enqueues the request, otherwise re-uses it. If both
> (ALSA and USB) frameworks calls this function at the same time, the
> kfifo_seek() will return the same usb_request, which will cause a race
> condition.
>
> To solve this problem a syncronization mechanism is necessary. In this case it
> is used a spinlock since f_midi_transmit is also called by usb_request->complete
> callback in interrupt context.
>
> On benchmarks realized by me, spinlocks were more efficient then scheduling
> the f_midi_transmit tasklet in process context and using a mutex to
> synchronize. Also it performs better then previous implementation that
> allocated a usb_request for every new transmit made.
behaves better in what way ? Also, previous implementation would not
suffer from this concurrency problem, right ?
--
balbi
Felipe Ferreri Tonello wrote:
> On 03/03/16 11:38, Clemens Ladisch wrote:
>> But in what way was the old state machine not "proper"?
>
> Because it didn't reflect all the correct and possible MIDI states
The whole point of the one-byte real-time messages is that they do not
affect the parsing of the surrounding MIDI stream. So not making them
part of the state machine is the proper way of handling them. (Also
see the flowchart in appendix A of the spec.)
> This patch doesn't change any functionality. But the important thing
> here is that it improves the driver maintainability [...]
Then I won't get in the way of this driver's maintainer.
Regards,
Clemens
Hi Clemens,
On March 4, 2016 8:07:40 AM GMT+00:00, Clemens Ladisch <[email protected]> wrote:
>Felipe Ferreri Tonello wrote:
>> On 03/03/16 11:38, Clemens Ladisch wrote:
>>> But in what way was the old state machine not "proper"?
>>
>> Because it didn't reflect all the correct and possible MIDI states
>
>The whole point of the one-byte real-time messages is that they do not
>affect the parsing of the surrounding MIDI stream. So not making them
>part of the state machine is the proper way of handling them. (Also
>see the flowchart in appendix A of the spec.)
I really don't get your point. So why do we have a state machine at all?
>
>> This patch doesn't change any functionality. But the important thing
>> here is that it improves the driver maintainability [...]
>
>Then I won't get in the way of this driver's maintainer.
Clemens, I really value your feedback. I just want to understand what's the problem of this patch.
Felipe
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Hi Balbi,
On March 4, 2016 7:13:05 AM GMT+00:00, Felipe Balbi <[email protected]> wrote:
>"Felipe F. Tonello" <[email protected]> writes:
>> [ text/plain ]
>> Signed-off-by: Felipe F. Tonello <[email protected]>
>
>no commit log == no commit
Got it.
>
>> ---
>> drivers/usb/gadget/function/f_midi.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c
>b/drivers/usb/gadget/function/f_midi.c
>> index 9a9e6112e224..5c7f5c780fda 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -5,6 +5,9 @@
>> * Developed for Thumtronics by Grey Innovation
>> * Ben Williamson <[email protected]>
>> *
>> + * Copyright (C) 2015,2016 ROLI Ltd.
>> + * Felipe F. Tonello <[email protected]>
>
>Did you check with your company's lawyer that your changes are enough
>to
>justify a copyright ?
Yes. Specially if that state machine refractor gets approved. TBH I can't see it won't.
Thanks,
Felipe
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Hi Balbi,
On March 4, 2016 7:11:30 AM GMT+00:00, Felipe Balbi <[email protected]> wrote:
>
>Hi,
>
>"Felipe F. Tonello" <[email protected]> writes:
>> [ text/plain ]
>> Patches are pretty much self-described.
>>
>> Patch 1 is revised from comments.
>
>you really need to describe what you changed. This also should have v2
>on subject line.
Right. I didn't in this case because I sent this patch previously a while ago right before you changed employer.
>
>I guess it's too late to get this in v4.6 merge window as I'm already
>applying the last few patches and plan to send a pull request in a few
>minutes.
That's fine I won't be able to rework the comments before Monday anyway.
Thanks,
Felipe
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Felipe Ferreri Tonello wrote:
> On March 4, 2016 8:07:40 AM GMT+00:00, Clemens Ladisch <[email protected]> wrote:
>> Felipe Ferreri Tonello wrote:
>>> On 03/03/16 11:38, Clemens Ladisch wrote:
>>>> But in what way was the old state machine not "proper"?
>>>
>>> Because it didn't reflect all the correct and possible MIDI states
>>
>> The whole point of the one-byte real-time messages is that they do not
>> affect the parsing of the surrounding MIDI stream. So not making them
>> part of the state machine is the proper way of handling them. (Also
>> see the flowchart in appendix A of the spec.)
>
> I really don't get your point. So why do we have a state machine at all?
To parse all the other messages.
Regards,
Clemens
Hi Balbi,
On March 4, 2016 7:16:42 AM GMT+00:00, Felipe Balbi <[email protected]> wrote:
>
>Hi,
>
>"Felipe F. Tonello" <[email protected]> writes:
>> [ text/plain ]
>> This gadget uses a bmAttributes and MaxPower that requires the USB
>bus to be
>> powered from the host, which is not correct because this
>configuration is device
>> specific, not a USB-MIDI requirement.
>>
>> This patch adds two modules parameters, bmAttributes and MaxPower,
>that allows
>> the user to set those flags. The default values are what the gadget
>used to use
>> for backward compatibility.
>>
>> Signed-off-by: Felipe F. Tonello <[email protected]>
>> ---
>> drivers/usb/gadget/legacy/gmidi.c | 14 ++++++++++++--
>> 1 file changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/legacy/gmidi.c
>b/drivers/usb/gadget/legacy/gmidi.c
>> index fc2ac150f5ff..0553435cc360 100644
>> --- a/drivers/usb/gadget/legacy/gmidi.c
>> +++ b/drivers/usb/gadget/legacy/gmidi.c
>> @@ -63,6 +63,14 @@ static unsigned int out_ports = 1;
>> module_param(out_ports, uint, S_IRUGO);
>> MODULE_PARM_DESC(out_ports, "Number of MIDI output ports");
>>
>> +static unsigned int bmAttributes = USB_CONFIG_ATT_ONE;
>> +module_param(bmAttributes, uint, S_IRUGO);
>> +MODULE_PARM_DESC(bmAttributes, "Configuration Descriptor's
>bmAttributes parameter");
>> +
>> +static unsigned int MaxPower = CONFIG_USB_GADGET_VBUS_DRAW;
>> +module_param(MaxPower, uint, S_IRUGO);
>> +MODULE_PARM_DESC(MaxPower, "Used to calculate Configuration
>Descriptor's bMaxPower parameter");
>
>you didn't run checkpatch, did you ? Also, are you sure you will need
>to
>change this by simply reloading the module ? I'm not convinced.
Yes I always run checkpatch :)
What do you mean by reloading the module?
>
>> @@ -119,8 +127,8 @@ static struct usb_configuration midi_config = {
>> .label = "MIDI Gadget",
>> .bConfigurationValue = 1,
>> /* .iConfiguration = DYNAMIC */
>> - .bmAttributes = USB_CONFIG_ATT_ONE,
>
>nack, nackety nack nack nack :-)
>
>USB_CONFIG_ATT_ONE *must* always be set. With your module parameter you
>give users the oportunity to violate USB spec.
You are right. It needs to check the value before it assigns to bmAttributes.
Felipe
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Hi Balbi,
On March 4, 2016 7:20:10 AM GMT+00:00, Felipe Balbi <[email protected]> wrote:
>
>Hi,
>
>"Felipe F. Tonello" <[email protected]> writes:
>> [ text/plain ]
>> Since f_midi_transmit is called by both ALSA and USB frameworks, it
>can
>> potentially cause a race condition between both calls. This is bad
>because the
>> way f_midi_transmit is implemented can't handle concurrent calls.
>This is due
>> to the fact that the usb request fifo looks for the next element and
>only if
>> it has data to process it enqueues the request, otherwise re-uses it.
>If both
>> (ALSA and USB) frameworks calls this function at the same time, the
>> kfifo_seek() will return the same usb_request, which will cause a
>race
>> condition.
>>
>> To solve this problem a syncronization mechanism is necessary. In
>this case it
>> is used a spinlock since f_midi_transmit is also called by
>usb_request->complete
>> callback in interrupt context.
>>
>> On benchmarks realized by me, spinlocks were more efficient then
>scheduling
>> the f_midi_transmit tasklet in process context and using a mutex to
>> synchronize. Also it performs better then previous implementation
>that
>> allocated a usb_request for every new transmit made.
>
>behaves better in what way ? Also, previous implementation would not
>suffer from this concurrency problem, right ?
The spin lock is faster than allocating usb requests all the time, even if the udc uses da for it.
That's true it wasn't necessary to put a spin lock in the gadget driver because the udc driver does it when allocating a new request.
Felipe
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Wed, Mar 02 2016, Felipe F. Tonello wrote:
> Signed-off-by: Felipe F. Tonello <[email protected]>
> ---
> drivers/usb/gadget/function/f_midi.c | 77 +++++++++++++++++++-----------------
> 1 file changed, 40 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_midi.c b/drivers/usb/gadget/function/f_midi.c
> index 8475e3dc82d4..9a9e6112e224 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -1,5 +1,5 @@
> /*
> - * f_midi.c -- USB MIDI class function driver
> + * f_midi.c -- USB-MIDI class function driver
> *
> * Copyright (C) 2006 Thumtronics Pty Ltd.
> * Developed for Thumtronics by Grey Innovation
> @@ -16,7 +16,7 @@
> * Copyright (C) 2006 Thumtronics Pty Ltd.
> * Ben Williamson <[email protected]>
> *
> - * Licensed under the GPL-2 or later.
> + * Licensed under the GPLv2.
Any particular reason to do that?
> */
>
> #include <linux/kernel.h>
> @@ -41,8 +41,8 @@
> MODULE_AUTHOR("Ben Williamson");
> MODULE_LICENSE("GPL v2");
>
> -static const char f_midi_shortname[] = "f_midi";
> -static const char f_midi_longname[] = "MIDI Gadget";
> +static const char f_midi_shortname[] = "f_midi";
> +static const char f_midi_longname[] = "MIDI Gadget";
>
> /*
> * We can only handle 16 cables on one single endpoint, as cable numbers are
> @@ -78,28 +78,31 @@ struct gmidi_in_port {
> };
>
> struct f_midi {
> - struct usb_function func;
> - struct usb_gadget *gadget;
> - struct usb_ep *in_ep, *out_ep;
> - struct snd_card *card;
> - struct snd_rawmidi *rmidi;
> - u8 ms_id;
> -
> - struct snd_rawmidi_substream *out_substream[MAX_PORTS];
> -
> - unsigned long out_triggered;
> - struct tasklet_struct tasklet;
> + struct usb_function func;
> + struct usb_gadget *gadget;
> + struct usb_ep *in_ep, *out_ep;
> + u8 ms_id;
> + unsigned long out_triggered;
> unsigned int in_ports;
> unsigned int out_ports;
> - int index;
> - char *id;
> - unsigned int buflen, qlen;
> + unsigned int buflen;
> + unsigned int qlen;
> + unsigned int len;
> +
> /* This fifo is used as a buffer ring for pre-allocated IN usb_requests */
> DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
> spinlock_t transmit_lock;
> +
> + /* ALSA stuff */
> + struct snd_card *card;
> + struct snd_rawmidi *rmidi;
> + struct snd_rawmidi_substream *out_substream[MAX_PORTS];
> + struct tasklet_struct tasklet;
> unsigned int in_last_port;
> + int index;
> + char *id;
>
> - struct gmidi_in_port in_ports_array[/* in_ports */];
> + struct gmidi_in_port in_ports_array[/* in_ports */];
> };
>
> static inline struct f_midi *func_to_midi(struct usb_function *f)
> @@ -191,7 +194,7 @@ static struct usb_ms_endpoint_descriptor_16 ms_in_desc = {
>
> /* string IDs are assigned dynamically */
>
> -#define STRING_FUNC_IDX 0
> +#define STRING_FUNC_IDX 0
>
> static struct usb_string midi_string_defs[] = {
> [STRING_FUNC_IDX].s = "MIDI function",
> @@ -199,7 +202,7 @@ static struct usb_string midi_string_defs[] = {
> };
>
> static struct usb_gadget_strings midi_stringtab = {
> - .language = 0x0409, /* en-us */
> + .language = 0x0409, /* en-us */
> .strings = midi_string_defs,
> };
>
> @@ -409,7 +412,7 @@ static int f_midi_snd_free(struct snd_device *device)
> }
>
> /*
> - * Converts MIDI commands to USB MIDI packets.
> + * 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)
> @@ -956,15 +959,15 @@ static int f_midi_bind(struct usb_configuration *c, struct usb_function *f)
> in_emb->iJack = 0;
> midi_function[i++] = (struct usb_descriptor_header *) in_emb;
>
> - out_ext->bLength = USB_DT_MIDI_OUT_SIZE(1);
> - out_ext->bDescriptorType = USB_DT_CS_INTERFACE;
> - out_ext->bDescriptorSubtype = USB_MS_MIDI_OUT_JACK;
> - out_ext->bJackType = USB_MS_EXTERNAL;
> - out_ext->bJackID = jack++;
> - out_ext->bNrInputPins = 1;
> - out_ext->iJack = 0;
> - out_ext->pins[0].baSourceID = in_emb->bJackID;
> - out_ext->pins[0].baSourcePin = 1;
> + out_ext->bLength = USB_DT_MIDI_OUT_SIZE(1);
> + out_ext->bDescriptorType = USB_DT_CS_INTERFACE;
> + out_ext->bDescriptorSubtype = USB_MS_MIDI_OUT_JACK;
> + out_ext->bJackType = USB_MS_EXTERNAL;
> + out_ext->bJackID = jack++;
> + out_ext->bNrInputPins = 1;
> + out_ext->iJack = 0;
> + out_ext->pins[0].baSourceID = in_emb->bJackID;
> + out_ext->pins[0].baSourcePin = 1;
> midi_function[i++] = (struct usb_descriptor_header *) out_ext;
>
> /* link it to the endpoint */
> @@ -1251,12 +1254,12 @@ static struct usb_function *f_midi_alloc(struct usb_function_instance *fi)
> status = -ENOMEM;
> goto setup_fail;
> }
> - midi->in_ports = opts->in_ports;
> - midi->out_ports = opts->out_ports;
> - midi->index = opts->index;
> - midi->buflen = opts->buflen;
> - midi->qlen = opts->qlen;
> - midi->in_last_port = 0;
> + midi->in_ports = opts->in_ports;
> + midi->out_ports = opts->out_ports;
> + midi->index = opts->index;
> + midi->buflen = opts->buflen;
> + midi->qlen = opts->qlen;
> + midi->in_last_port = 0;
I don’t understand this patch. You seem to be opposed to lining up
field names in a structure, but you are explicitly adding lining up to
assignment? What is going on here? IS this patch really improving
things?
>
> status = kfifo_alloc(&midi->in_req_fifo, midi->qlen, GFP_KERNEL);
> if (status)
> --
> 2.7.2
>
--
Best regards
ミハウ “????????????????86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»
Hi Michal,
On March 4, 2016 7:17:31 PM GMT+00:00, Michal Nazarewicz <[email protected]> wrote:
>On Wed, Mar 02 2016, Felipe F. Tonello wrote:
>> Signed-off-by: Felipe F. Tonello <[email protected]>
>> ---
>> drivers/usb/gadget/function/f_midi.c | 77
>+++++++++++++++++++-----------------
>> 1 file changed, 40 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c
>b/drivers/usb/gadget/function/f_midi.c
>> index 8475e3dc82d4..9a9e6112e224 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -1,5 +1,5 @@
>> /*
>> - * f_midi.c -- USB MIDI class function driver
>> + * f_midi.c -- USB-MIDI class function driver
>> *
>> * Copyright (C) 2006 Thumtronics Pty Ltd.
>> * Developed for Thumtronics by Grey Innovation
>> @@ -16,7 +16,7 @@
>> * Copyright (C) 2006 Thumtronics Pty Ltd.
>> * Ben Williamson <[email protected]>
>> *
>> - * Licensed under the GPL-2 or later.
>> + * Licensed under the GPLv2.
>
>Any particular reason to do that?
Because the kernel is v2 only and not later.
>
>> */
>>
>> #include <linux/kernel.h>
>> @@ -41,8 +41,8 @@
>> MODULE_AUTHOR("Ben Williamson");
>> MODULE_LICENSE("GPL v2");
>>
>> -static const char f_midi_shortname[] = "f_midi";
>> -static const char f_midi_longname[] = "MIDI Gadget";
>> +static const char f_midi_shortname[] = "f_midi";
>> +static const char f_midi_longname[] = "MIDI Gadget";
>>
>> /*
>> * We can only handle 16 cables on one single endpoint, as cable
>numbers are
>> @@ -78,28 +78,31 @@ struct gmidi_in_port {
>> };
>>
>> struct f_midi {
>> - struct usb_function func;
>> - struct usb_gadget *gadget;
>> - struct usb_ep *in_ep, *out_ep;
>> - struct snd_card *card;
>> - struct snd_rawmidi *rmidi;
>> - u8 ms_id;
>> -
>> - struct snd_rawmidi_substream *out_substream[MAX_PORTS];
>> -
>> - unsigned long out_triggered;
>> - struct tasklet_struct tasklet;
>> + struct usb_function func;
>> + struct usb_gadget *gadget;
>> + struct usb_ep *in_ep, *out_ep;
>> + u8 ms_id;
>> + unsigned long out_triggered;
>> unsigned int in_ports;
>> unsigned int out_ports;
>> - int index;
>> - char *id;
>> - unsigned int buflen, qlen;
>> + unsigned int buflen;
>> + unsigned int qlen;
>> + unsigned int len;
>> +
>> /* This fifo is used as a buffer ring for pre-allocated IN
>usb_requests */
>> DECLARE_KFIFO_PTR(in_req_fifo, struct usb_request *);
>> spinlock_t transmit_lock;
>> +
>> + /* ALSA stuff */
>> + struct snd_card *card;
>> + struct snd_rawmidi *rmidi;
>> + struct snd_rawmidi_substream *out_substream[MAX_PORTS];
>> + struct tasklet_struct tasklet;
>> unsigned int in_last_port;
>> + int index;
>> + char *id;
>>
>> - struct gmidi_in_port in_ports_array[/* in_ports */];
>> + struct gmidi_in_port in_ports_array[/* in_ports */];
>> };
>>
>> static inline struct f_midi *func_to_midi(struct usb_function *f)
>> @@ -191,7 +194,7 @@ static struct usb_ms_endpoint_descriptor_16
>ms_in_desc = {
>>
>> /* string IDs are assigned dynamically */
>>
>> -#define STRING_FUNC_IDX 0
>> +#define STRING_FUNC_IDX 0
>>
>> static struct usb_string midi_string_defs[] = {
>> [STRING_FUNC_IDX].s = "MIDI function",
>> @@ -199,7 +202,7 @@ static struct usb_string midi_string_defs[] = {
>> };
>>
>> static struct usb_gadget_strings midi_stringtab = {
>> - .language = 0x0409, /* en-us */
>> + .language = 0x0409, /* en-us */
>> .strings = midi_string_defs,
>> };
>>
>> @@ -409,7 +412,7 @@ static int f_midi_snd_free(struct snd_device
>*device)
>> }
>>
>> /*
>> - * Converts MIDI commands to USB MIDI packets.
>> + * 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)
>> @@ -956,15 +959,15 @@ static int f_midi_bind(struct usb_configuration
>*c, struct usb_function *f)
>> in_emb->iJack = 0;
>> midi_function[i++] = (struct usb_descriptor_header *) in_emb;
>>
>> - out_ext->bLength = USB_DT_MIDI_OUT_SIZE(1);
>> - out_ext->bDescriptorType = USB_DT_CS_INTERFACE;
>> - out_ext->bDescriptorSubtype = USB_MS_MIDI_OUT_JACK;
>> - out_ext->bJackType = USB_MS_EXTERNAL;
>> - out_ext->bJackID = jack++;
>> - out_ext->bNrInputPins = 1;
>> - out_ext->iJack = 0;
>> - out_ext->pins[0].baSourceID = in_emb->bJackID;
>> - out_ext->pins[0].baSourcePin = 1;
>> + out_ext->bLength = USB_DT_MIDI_OUT_SIZE(1);
>> + out_ext->bDescriptorType = USB_DT_CS_INTERFACE;
>> + out_ext->bDescriptorSubtype = USB_MS_MIDI_OUT_JACK;
>> + out_ext->bJackType = USB_MS_EXTERNAL;
>> + out_ext->bJackID = jack++;
>> + out_ext->bNrInputPins = 1;
>> + out_ext->iJack = 0;
>> + out_ext->pins[0].baSourceID = in_emb->bJackID;
>> + out_ext->pins[0].baSourcePin = 1;
>> midi_function[i++] = (struct usb_descriptor_header *) out_ext;
>>
>> /* link it to the endpoint */
>> @@ -1251,12 +1254,12 @@ static struct usb_function
>*f_midi_alloc(struct usb_function_instance *fi)
>> status = -ENOMEM;
>> goto setup_fail;
>> }
>> - midi->in_ports = opts->in_ports;
>> - midi->out_ports = opts->out_ports;
>> - midi->index = opts->index;
>> - midi->buflen = opts->buflen;
>> - midi->qlen = opts->qlen;
>> - midi->in_last_port = 0;
>> + midi->in_ports = opts->in_ports;
>> + midi->out_ports = opts->out_ports;
>> + midi->index = opts->index;
>> + midi->buflen = opts->buflen;
>> + midi->qlen = opts->qlen;
>> + midi->in_last_port = 0;
>
>I don’t understand this patch. You seem to be opposed to lining up
>field names in a structure, but you are explicitly adding lining up to
>assignment? What is going on here? IS this patch really improving
>things?
I just tried to make this driver more consistent with the coding style used across the kernel. That's it.
>
>>
>> status = kfifo_alloc(&midi->in_req_fifo, midi->qlen, GFP_KERNEL);
>> if (status)
>> --
>> 2.7.2
>>
Felipe
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
>> On Wed, Mar 02 2016, Felipe F. Tonello wrote:
>>> @@ -16,7 +16,7 @@
>>> * Copyright (C) 2006 Thumtronics Pty Ltd.
>>> * Ben Williamson <[email protected]>
>>> *
>>> - * Licensed under the GPL-2 or later.
>>> + * Licensed under the GPLv2.
> On March 4, 2016 7:17:31 PM GMT+00:00, Michal Nazarewicz <[email protected]> wrote:
>> Any particular reason to do that?
On Fri, Mar 04 2016, Felipe Ferreri Tonello wrote:
> Because the kernel is v2 only and not later.
Linux as a whole is GPLv2 only, but that doesn’t necessarily mean that
parts of it cannot be dual licensed (or GPLv2+). It’s safer to leave
copyright noticed clear unless you explicitly want your contribution be
GPLv2 only which brings the whole file GPLv2 only.
> I just tried to make this driver more consistent with the coding style
> used across the kernel. That's it.
Column alignment of field names or RHS of assignment operators is quite
inconsistent already within drivers/usb/gadget/ which is why I’m
concerned whether this is really helping.
Anyway, I actually don’t care much, just adding my two rappen.
--
Best regards
ミハウ “????????????????86” ナザレヴイツ
«If at first you don’t succeed, give up skydiving»
On Sat, Mar 05, 2016 at 11:28:45AM -0500, Michal Nazarewicz wrote:
> >> On Wed, Mar 02 2016, Felipe F. Tonello wrote:
> >>> @@ -16,7 +16,7 @@
> >>> * Copyright (C) 2006 Thumtronics Pty Ltd.
> >>> * Ben Williamson <[email protected]>
> >>> *
> >>> - * Licensed under the GPL-2 or later.
> >>> + * Licensed under the GPLv2.
>
> > On March 4, 2016 7:17:31 PM GMT+00:00, Michal Nazarewicz <[email protected]> wrote:
> >> Any particular reason to do that?
>
> On Fri, Mar 04 2016, Felipe Ferreri Tonello wrote:
> > Because the kernel is v2 only and not later.
>
> Linux as a whole is GPLv2 only, but that doesn’t necessarily mean that
> parts of it cannot be dual licensed (or GPLv2+). It’s safer to leave
> copyright noticed clear unless you explicitly want your contribution be
> GPLv2 only which brings the whole file GPLv2 only.
But you can't change the license of someone else's code, which is what
is happening here. Felipe T, you can't do that at all unless you want
to get into big trouble, please consult a lawyer for all of the gory
details.
greg k-h
Hi Greg,
On March 5, 2016 7:39:13 PM GMT+00:00, Greg KH <[email protected]> wrote:
>On Sat, Mar 05, 2016 at 11:28:45AM -0500, Michal Nazarewicz wrote:
>> >> On Wed, Mar 02 2016, Felipe F. Tonello wrote:
>> >>> @@ -16,7 +16,7 @@
>> >>> * Copyright (C) 2006 Thumtronics Pty Ltd.
>> >>> * Ben Williamson <[email protected]>
>> >>> *
>> >>> - * Licensed under the GPL-2 or later.
>> >>> + * Licensed under the GPLv2.
>>
>> > On March 4, 2016 7:17:31 PM GMT+00:00, Michal Nazarewicz
><[email protected]> wrote:
>> >> Any particular reason to do that?
>>
>> On Fri, Mar 04 2016, Felipe Ferreri Tonello wrote:
>> > Because the kernel is v2 only and not later.
>>
>> Linux as a whole is GPLv2 only, but that doesn’t necessarily mean
>that
>> parts of it cannot be dual licensed (or GPLv2+). It’s safer to leave
>> copyright noticed clear unless you explicitly want your contribution
>be
>> GPLv2 only which brings the whole file GPLv2 only.
>
>But you can't change the license of someone else's code, which is what
>is happening here. Felipe T, you can't do that at all unless you want
>to get into big trouble, please consult a lawyer for all of the gory
>details.
Thanks for letting me know. TBH, I had no idea about it.
Felipe
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Hi Michal,
On March 5, 2016 4:28:45 PM GMT+00:00, Michal Nazarewicz <[email protected]> wrote:
>>> On Wed, Mar 02 2016, Felipe F. Tonello wrote:
>>>> @@ -16,7 +16,7 @@
>>>> * Copyright (C) 2006 Thumtronics Pty Ltd.
>>>> * Ben Williamson <[email protected]>
>>>> *
>>>> - * Licensed under the GPL-2 or later.
>>>> + * Licensed under the GPLv2.
>
>> On March 4, 2016 7:17:31 PM GMT+00:00, Michal Nazarewicz
><[email protected]> wrote:
>>> Any particular reason to do that?
>
>On Fri, Mar 04 2016, Felipe Ferreri Tonello wrote:
>> Because the kernel is v2 only and not later.
>
>Linux as a whole is GPLv2 only, but that doesn’t necessarily mean that
>parts of it cannot be dual licensed (or GPLv2+). It’s safer to leave
>copyright noticed clear unless you explicitly want your contribution be
>GPLv2 only which brings the whole file GPLv2 only.
>
>> I just tried to make this driver more consistent with the coding
>style
>> used across the kernel. That's it.
>
>Column alignment of field names or RHS of assignment operators is quite
>inconsistent already within drivers/usb/gadget/ which is why I’m
>concerned whether this is really helping.
>
>Anyway, I actually don’t care much, just adding my two rappen.
Right, I am ok with Balbi completely ignoring this patch. But I prefer to have at least this driver consistent than nothing. Of course I'll remove the license change I made.
Felipe
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Sat, Mar 05, 2016 at 11:53:35PM +0000, Felipe Ferreri Tonello wrote:
> Hi Greg,
>
> On March 5, 2016 7:39:13 PM GMT+00:00, Greg KH <[email protected]> wrote:
> >On Sat, Mar 05, 2016 at 11:28:45AM -0500, Michal Nazarewicz wrote:
> >> >> On Wed, Mar 02 2016, Felipe F. Tonello wrote:
> >> >>> @@ -16,7 +16,7 @@
> >> >>> * Copyright (C) 2006 Thumtronics Pty Ltd.
> >> >>> * Ben Williamson <[email protected]>
> >> >>> *
> >> >>> - * Licensed under the GPL-2 or later.
> >> >>> + * Licensed under the GPLv2.
> >>
> >> > On March 4, 2016 7:17:31 PM GMT+00:00, Michal Nazarewicz
> ><[email protected]> wrote:
> >> >> Any particular reason to do that?
> >>
> >> On Fri, Mar 04 2016, Felipe Ferreri Tonello wrote:
> >> > Because the kernel is v2 only and not later.
> >>
> >> Linux as a whole is GPLv2 only, but that doesn’t necessarily mean
> >that
> >> parts of it cannot be dual licensed (or GPLv2+). It’s safer to leave
> >> copyright noticed clear unless you explicitly want your contribution
> >be
> >> GPLv2 only which brings the whole file GPLv2 only.
> >
> >But you can't change the license of someone else's code, which is what
> >is happening here. Felipe T, you can't do that at all unless you want
> >to get into big trouble, please consult a lawyer for all of the gory
> >details.
>
> Thanks for letting me know. TBH, I had no idea about it.
Never change a copyright or a license if you don't know exactly what you
are doing, or why you are doing it, and have consulted with a lawyer
beforehand. The issues here are real, don't take them lightly.
greg k-h
Hi,
(please break your lines at 80-characters, have a look at
Documentation/email-clients.txt if needed)
Felipe Ferreri Tonello <[email protected]> writes:
> [ text/plain ]
> Hi Balbi,
>
> On March 4, 2016 7:20:10 AM GMT+00:00, Felipe Balbi <[email protected]> wrote:
>>
>>Hi,
>>
>>"Felipe F. Tonello" <[email protected]> writes:
>>> [ text/plain ]
>>> Since f_midi_transmit is called by both ALSA and USB frameworks, it
>>can
>>> potentially cause a race condition between both calls. This is bad
>>because the
>>> way f_midi_transmit is implemented can't handle concurrent calls.
>>This is due
>>> to the fact that the usb request fifo looks for the next element and
>>only if
>>> it has data to process it enqueues the request, otherwise re-uses it.
>>If both
>>> (ALSA and USB) frameworks calls this function at the same time, the
>>> kfifo_seek() will return the same usb_request, which will cause a
>>race
>>> condition.
>>>
>>> To solve this problem a syncronization mechanism is necessary. In
>>this case it
>>> is used a spinlock since f_midi_transmit is also called by
>>usb_request->complete
>>> callback in interrupt context.
>>>
>>> On benchmarks realized by me, spinlocks were more efficient then
>>scheduling
>>> the f_midi_transmit tasklet in process context and using a mutex to
>>> synchronize. Also it performs better then previous implementation
>>that
>>> allocated a usb_request for every new transmit made.
>>
>>behaves better in what way ? Also, previous implementation would not
>>suffer from this concurrency problem, right ?
>
> The spin lock is faster than allocating usb requests all the time,
> even if the udc uses da for it.
did you measure ? Is the extra speed really necessary ? How did you
benchmark this ?
--
balbi
Hi,
Felipe Ferreri Tonello <[email protected]> writes:
> [ text/plain ]
> Hi Balbi,
>
> On March 4, 2016 7:16:42 AM GMT+00:00, Felipe Balbi <[email protected]> wrote:
>>
>>Hi,
>>
>>"Felipe F. Tonello" <[email protected]> writes:
>>> [ text/plain ]
>>> This gadget uses a bmAttributes and MaxPower that requires the USB
>>bus to be
>>> powered from the host, which is not correct because this
>>configuration is device
>>> specific, not a USB-MIDI requirement.
>>>
>>> This patch adds two modules parameters, bmAttributes and MaxPower,
>>that allows
>>> the user to set those flags. The default values are what the gadget
>>used to use
>>> for backward compatibility.
>>>
>>> Signed-off-by: Felipe F. Tonello <[email protected]>
>>> ---
>>> drivers/usb/gadget/legacy/gmidi.c | 14 ++++++++++++--
>>> 1 file changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/gadget/legacy/gmidi.c
>>b/drivers/usb/gadget/legacy/gmidi.c
>>> index fc2ac150f5ff..0553435cc360 100644
>>> --- a/drivers/usb/gadget/legacy/gmidi.c
>>> +++ b/drivers/usb/gadget/legacy/gmidi.c
>>> @@ -63,6 +63,14 @@ static unsigned int out_ports = 1;
>>> module_param(out_ports, uint, S_IRUGO);
>>> MODULE_PARM_DESC(out_ports, "Number of MIDI output ports");
>>>
>>> +static unsigned int bmAttributes = USB_CONFIG_ATT_ONE;
>>> +module_param(bmAttributes, uint, S_IRUGO);
>>> +MODULE_PARM_DESC(bmAttributes, "Configuration Descriptor's
>>bmAttributes parameter");
>>> +
>>> +static unsigned int MaxPower = CONFIG_USB_GADGET_VBUS_DRAW;
>>> +module_param(MaxPower, uint, S_IRUGO);
>>> +MODULE_PARM_DESC(MaxPower, "Used to calculate Configuration
>>Descriptor's bMaxPower parameter");
>>
>>you didn't run checkpatch, did you ? Also, are you sure you will need
>>to
>>change this by simply reloading the module ? I'm not convinced.
>
> Yes I always run checkpatch :)
do you really ? Why do you have a 98-character line, then ?
> What do you mean by reloading the module?
modprobe g_midi MaxPower=4
modprobe -r g_midi
modprobe g_midi MaxPower=10
I'm not convinced this is a valid use-case. Specially since you can just
provide several configurations and let the host choose the one it suits
it best.
>>> @@ -119,8 +127,8 @@ static struct usb_configuration midi_config = {
>>> .label = "MIDI Gadget",
>>> .bConfigurationValue = 1,
>>> /* .iConfiguration = DYNAMIC */
>>> - .bmAttributes = USB_CONFIG_ATT_ONE,
>>
>>nack, nackety nack nack nack :-)
>>
>>USB_CONFIG_ATT_ONE *must* always be set. With your module parameter you
>>give users the oportunity to violate USB spec.
>
> You are right. It needs to check the value before it assigns to
> bmAttributes.
why check ? You can just unconditionally or USB_CONFIG_ATT_ONE. In any
case, I'm not convinced this is necessary at all.
--
balbi
Hi,
Felipe Ferreri Tonello <[email protected]> writes:
> [ text/plain ]
> Hi Michal,
>
> On March 5, 2016 4:28:45 PM GMT+00:00, Michal Nazarewicz <[email protected]> wrote:
>>>> On Wed, Mar 02 2016, Felipe F. Tonello wrote:
>>>>> @@ -16,7 +16,7 @@
>>>>> * Copyright (C) 2006 Thumtronics Pty Ltd.
>>>>> * Ben Williamson <[email protected]>
>>>>> *
>>>>> - * Licensed under the GPL-2 or later.
>>>>> + * Licensed under the GPLv2.
>>
>>> On March 4, 2016 7:17:31 PM GMT+00:00, Michal Nazarewicz
>><[email protected]> wrote:
>>>> Any particular reason to do that?
>>
>>On Fri, Mar 04 2016, Felipe Ferreri Tonello wrote:
>>> Because the kernel is v2 only and not later.
>>
>>Linux as a whole is GPLv2 only, but that doesn’t necessarily mean that
>>parts of it cannot be dual licensed (or GPLv2+). It’s safer to leave
>>copyright noticed clear unless you explicitly want your contribution be
>>GPLv2 only which brings the whole file GPLv2 only.
>>
>>> I just tried to make this driver more consistent with the coding
>>style
>>> used across the kernel. That's it.
>>
>>Column alignment of field names or RHS of assignment operators is quite
>>inconsistent already within drivers/usb/gadget/ which is why I’m
>>concerned whether this is really helping.
>>
>>Anyway, I actually don’t care much, just adding my two rappen.
>
> Right, I am ok with Balbi completely ignoring this patch. But I prefer
> to have at least this driver consistent than nothing. Of course I'll
> remove the license change I made.
consistent in what way ?
--
balbi
Hi,
Felipe Ferreri Tonello <[email protected]> writes:
> [ text/plain ]
> Hi Balbi,
>
> On March 4, 2016 7:13:05 AM GMT+00:00, Felipe Balbi <[email protected]> wrote:
>>"Felipe F. Tonello" <[email protected]> writes:
>>> [ text/plain ]
>>> Signed-off-by: Felipe F. Tonello <[email protected]>
>>
>>no commit log == no commit
>
> Got it.
>
>>
>>> ---
>>> drivers/usb/gadget/function/f_midi.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/usb/gadget/function/f_midi.c
>>b/drivers/usb/gadget/function/f_midi.c
>>> index 9a9e6112e224..5c7f5c780fda 100644
>>> --- a/drivers/usb/gadget/function/f_midi.c
>>> +++ b/drivers/usb/gadget/function/f_midi.c
>>> @@ -5,6 +5,9 @@
>>> * Developed for Thumtronics by Grey Innovation
>>> * Ben Williamson <[email protected]>
>>> *
>>> + * Copyright (C) 2015,2016 ROLI Ltd.
>>> + * Felipe F. Tonello <[email protected]>
>>
>>Did you check with your company's lawyer that your changes are enough
>>to
>>justify a copyright ?
>
> Yes. Specially if that state machine refractor gets approved. TBH I
> can't see it won't.
okay, so did that same lawyer tell you to change the driver's license ?
--
balbi
Hi Balbi,
On 07/03/16 07:36, Felipe Balbi wrote:
>
> Hi,
>
> Felipe Ferreri Tonello <[email protected]> writes:
>> [ text/plain ]
>> Hi Balbi,
>>
>> On March 4, 2016 7:13:05 AM GMT+00:00, Felipe Balbi <[email protected]> wrote:
>>> "Felipe F. Tonello" <[email protected]> writes:
>>>> [ text/plain ]
>>>> Signed-off-by: Felipe F. Tonello <[email protected]>
>>>
>>> no commit log == no commit
>>
>> Got it.
>>
>>>
>>>> ---
>>>> drivers/usb/gadget/function/f_midi.c | 3 +++
>>>> 1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/gadget/function/f_midi.c
>>> b/drivers/usb/gadget/function/f_midi.c
>>>> index 9a9e6112e224..5c7f5c780fda 100644
>>>> --- a/drivers/usb/gadget/function/f_midi.c
>>>> +++ b/drivers/usb/gadget/function/f_midi.c
>>>> @@ -5,6 +5,9 @@
>>>> * Developed for Thumtronics by Grey Innovation
>>>> * Ben Williamson <[email protected]>
>>>> *
>>>> + * Copyright (C) 2015,2016 ROLI Ltd.
>>>> + * Felipe F. Tonello <[email protected]>
>>>
>>> Did you check with your company's lawyer that your changes are enough
>>> to
>>> justify a copyright ?
>>
>> Yes. Specially if that state machine refractor gets approved. TBH I
>> can't see it won't.
>
> okay, so did that same lawyer tell you to change the driver's license ?
>
No. That was my bad call. TBH I really don't care about this copyright.
You can just ignore this patch and patch 4.
Thanks
Felipe
Hi Balbi,
On 07/03/16 07:32, Felipe Balbi wrote:
>
> Hi,
>
> (please break your lines at 80-characters, have a look at
> Documentation/email-clients.txt if needed)
>
> Felipe Ferreri Tonello <[email protected]> writes:
>> [ text/plain ]
>> Hi Balbi,
>>
>> On March 4, 2016 7:20:10 AM GMT+00:00, Felipe Balbi <[email protected]> wrote:
>>>
>>> Hi,
>>>
>>> "Felipe F. Tonello" <[email protected]> writes:
>>>> [ text/plain ]
>>>> Since f_midi_transmit is called by both ALSA and USB frameworks, it
>>> can
>>>> potentially cause a race condition between both calls. This is bad
>>> because the
>>>> way f_midi_transmit is implemented can't handle concurrent calls.
>>> This is due
>>>> to the fact that the usb request fifo looks for the next element and
>>> only if
>>>> it has data to process it enqueues the request, otherwise re-uses it.
>>> If both
>>>> (ALSA and USB) frameworks calls this function at the same time, the
>>>> kfifo_seek() will return the same usb_request, which will cause a
>>> race
>>>> condition.
>>>>
>>>> To solve this problem a syncronization mechanism is necessary. In
>>> this case it
>>>> is used a spinlock since f_midi_transmit is also called by
>>> usb_request->complete
>>>> callback in interrupt context.
>>>>
>>>> On benchmarks realized by me, spinlocks were more efficient then
>>> scheduling
>>>> the f_midi_transmit tasklet in process context and using a mutex to
>>>> synchronize. Also it performs better then previous implementation
>>> that
>>>> allocated a usb_request for every new transmit made.
>>>
>>> behaves better in what way ? Also, previous implementation would not
>>> suffer from this concurrency problem, right ?
>>
>> The spin lock is faster than allocating usb requests all the time,
>> even if the udc uses da for it.
>
> did you measure ? Is the extra speed really necessary ? How did you
> benchmark this ?
Yes I did measure and it was not that significant. This is not about
speed. There was a bug in that approach that I already explained on that
patch, which was approved and applied BTW.
Any way, this spinlock should've been there since that patch but I
couldn't really trigger this problem without a stress test.
So, this patch fixes a bug in the current implementation.
Felipe
Hi Balbi,
On 07/03/16 07:35, Felipe Balbi wrote:
>
> Hi,
>
> Felipe Ferreri Tonello <[email protected]> writes:
>> [ text/plain ]
>> Hi Michal,
>>
>> On March 5, 2016 4:28:45 PM GMT+00:00, Michal Nazarewicz <[email protected]> wrote:
>>>>> On Wed, Mar 02 2016, Felipe F. Tonello wrote:
>>>>>> @@ -16,7 +16,7 @@
>>>>>> * Copyright (C) 2006 Thumtronics Pty Ltd.
>>>>>> * Ben Williamson <[email protected]>
>>>>>> *
>>>>>> - * Licensed under the GPL-2 or later.
>>>>>> + * Licensed under the GPLv2.
>>>
>>>> On March 4, 2016 7:17:31 PM GMT+00:00, Michal Nazarewicz
>>> <[email protected]> wrote:
>>>>> Any particular reason to do that?
>>>
>>> On Fri, Mar 04 2016, Felipe Ferreri Tonello wrote:
>>>> Because the kernel is v2 only and not later.
>>>
>>> Linux as a whole is GPLv2 only, but that doesn’t necessarily mean that
>>> parts of it cannot be dual licensed (or GPLv2+). It’s safer to leave
>>> copyright noticed clear unless you explicitly want your contribution be
>>> GPLv2 only which brings the whole file GPLv2 only.
>>>
>>>> I just tried to make this driver more consistent with the coding
>>> style
>>>> used across the kernel. That's it.
>>>
>>> Column alignment of field names or RHS of assignment operators is quite
>>> inconsistent already within drivers/usb/gadget/ which is why I’m
>>> concerned whether this is really helping.
>>>
>>> Anyway, I actually don’t care much, just adding my two rappen.
>>
>> Right, I am ok with Balbi completely ignoring this patch. But I prefer
>> to have at least this driver consistent than nothing. Of course I'll
>> remove the license change I made.
>
> consistent in what way ?
Source-code.
The goal of this patch is to update this driver coding style to promote
consistency, readability, and maintainability based on the Linux coding
style.
If this patch does not achieving that or if that is not necessary, than
just ignore this patch.
Thanks,
Felipe
Hi Balbi,
On 07/03/16 07:34, Felipe Balbi wrote:
>
> Hi,
>
> Felipe Ferreri Tonello <[email protected]> writes:
>> [ text/plain ]
>> Hi Balbi,
>>
>> On March 4, 2016 7:16:42 AM GMT+00:00, Felipe Balbi <[email protected]> wrote:
>>>
>>> Hi,
>>>
>>> "Felipe F. Tonello" <[email protected]> writes:
>>>> [ text/plain ]
>>>> This gadget uses a bmAttributes and MaxPower that requires the USB
>>> bus to be
>>>> powered from the host, which is not correct because this
>>> configuration is device
>>>> specific, not a USB-MIDI requirement.
>>>>
>>>> This patch adds two modules parameters, bmAttributes and MaxPower,
>>> that allows
>>>> the user to set those flags. The default values are what the gadget
>>> used to use
>>>> for backward compatibility.
>>>>
>>>> Signed-off-by: Felipe F. Tonello <[email protected]>
>>>> ---
>>>> drivers/usb/gadget/legacy/gmidi.c | 14 ++++++++++++--
>>>> 1 file changed, 12 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/gadget/legacy/gmidi.c
>>> b/drivers/usb/gadget/legacy/gmidi.c
>>>> index fc2ac150f5ff..0553435cc360 100644
>>>> --- a/drivers/usb/gadget/legacy/gmidi.c
>>>> +++ b/drivers/usb/gadget/legacy/gmidi.c
>>>> @@ -63,6 +63,14 @@ static unsigned int out_ports = 1;
>>>> module_param(out_ports, uint, S_IRUGO);
>>>> MODULE_PARM_DESC(out_ports, "Number of MIDI output ports");
>>>>
>>>> +static unsigned int bmAttributes = USB_CONFIG_ATT_ONE;
>>>> +module_param(bmAttributes, uint, S_IRUGO);
>>>> +MODULE_PARM_DESC(bmAttributes, "Configuration Descriptor's
>>> bmAttributes parameter");
>>>> +
>>>> +static unsigned int MaxPower = CONFIG_USB_GADGET_VBUS_DRAW;
>>>> +module_param(MaxPower, uint, S_IRUGO);
>>>> +MODULE_PARM_DESC(MaxPower, "Used to calculate Configuration
>>> Descriptor's bMaxPower parameter");
>>>
>>> you didn't run checkpatch, did you ? Also, are you sure you will need
>>> to
>>> change this by simply reloading the module ? I'm not convinced.
>>
>> Yes I always run checkpatch :)
>
> do you really ? Why do you have a 98-character line, then ?
>
>> What do you mean by reloading the module?
>
> modprobe g_midi MaxPower=4
> modprobe -r g_midi
> modprobe g_midi MaxPower=10
>
> I'm not convinced this is a valid use-case. Specially since you can just
> provide several configurations and let the host choose the one it suits
> it best.
Ok, I will further test it.
>
>>>> @@ -119,8 +127,8 @@ static struct usb_configuration midi_config = {
>>>> .label = "MIDI Gadget",
>>>> .bConfigurationValue = 1,
>>>> /* .iConfiguration = DYNAMIC */
>>>> - .bmAttributes = USB_CONFIG_ATT_ONE,
>>>
>>> nack, nackety nack nack nack :-)
>>>
>>> USB_CONFIG_ATT_ONE *must* always be set. With your module parameter you
>>> give users the oportunity to violate USB spec.
>>
>> You are right. It needs to check the value before it assigns to
>> bmAttributes.
>
> why check ? You can just unconditionally or USB_CONFIG_ATT_ONE. In any
> case, I'm not convinced this is necessary at all.
Right.
This is necessary because this driver is actually wrong in which is
asking for the host to power itself. This is not specified on USB-MIDI
specification, neither makes any sense since this configuration is
device specific.
What is your suggestion to make it configurable? Maybe at compile-time?
I really don't know what is the best solution if this is not something
you like it.
Felipe
Hi,
Felipe Ferreri Tonello <[email protected]> writes:
>>>> "Felipe F. Tonello" <[email protected]> writes:
>>>>> [ text/plain ]
>>>>> This gadget uses a bmAttributes and MaxPower that requires the USB
>>>> bus to be
>>>>> powered from the host, which is not correct because this
>>>> configuration is device
>>>>> specific, not a USB-MIDI requirement.
>>>>>
>>>>> This patch adds two modules parameters, bmAttributes and MaxPower,
>>>> that allows
>>>>> the user to set those flags. The default values are what the gadget
>>>> used to use
>>>>> for backward compatibility.
>>>>>
>>>>> Signed-off-by: Felipe F. Tonello <[email protected]>
>>>>> ---
>>>>> drivers/usb/gadget/legacy/gmidi.c | 14 ++++++++++++--
>>>>> 1 file changed, 12 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/gadget/legacy/gmidi.c
>>>> b/drivers/usb/gadget/legacy/gmidi.c
>>>>> index fc2ac150f5ff..0553435cc360 100644
>>>>> --- a/drivers/usb/gadget/legacy/gmidi.c
>>>>> +++ b/drivers/usb/gadget/legacy/gmidi.c
>>>>> @@ -63,6 +63,14 @@ static unsigned int out_ports = 1;
>>>>> module_param(out_ports, uint, S_IRUGO);
>>>>> MODULE_PARM_DESC(out_ports, "Number of MIDI output ports");
>>>>>
>>>>> +static unsigned int bmAttributes = USB_CONFIG_ATT_ONE;
>>>>> +module_param(bmAttributes, uint, S_IRUGO);
>>>>> +MODULE_PARM_DESC(bmAttributes, "Configuration Descriptor's
>>>> bmAttributes parameter");
>>>>> +
>>>>> +static unsigned int MaxPower = CONFIG_USB_GADGET_VBUS_DRAW;
>>>>> +module_param(MaxPower, uint, S_IRUGO);
>>>>> +MODULE_PARM_DESC(MaxPower, "Used to calculate Configuration
>>>> Descriptor's bMaxPower parameter");
>>>>
>>>> you didn't run checkpatch, did you ? Also, are you sure you will need
>>>> to
>>>> change this by simply reloading the module ? I'm not convinced.
>>>
>>> Yes I always run checkpatch :)
>>
>> do you really ? Why do you have a 98-character line, then ?
>>
>>> What do you mean by reloading the module?
>>
>> modprobe g_midi MaxPower=4
>> modprobe -r g_midi
>> modprobe g_midi MaxPower=10
>>
>> I'm not convinced this is a valid use-case. Specially since you can just
>> provide several configurations and let the host choose the one it suits
>> it best.
>
> Ok, I will further test it.
>
>>
>>>>> @@ -119,8 +127,8 @@ static struct usb_configuration midi_config = {
>>>>> .label = "MIDI Gadget",
>>>>> .bConfigurationValue = 1,
>>>>> /* .iConfiguration = DYNAMIC */
>>>>> - .bmAttributes = USB_CONFIG_ATT_ONE,
>>>>
>>>> nack, nackety nack nack nack :-)
>>>>
>>>> USB_CONFIG_ATT_ONE *must* always be set. With your module parameter you
>>>> give users the oportunity to violate USB spec.
>>>
>>> You are right. It needs to check the value before it assigns to
>>> bmAttributes.
>>
>> why check ? You can just unconditionally or USB_CONFIG_ATT_ONE. In any
>> case, I'm not convinced this is necessary at all.
>
> Right.
>
> This is necessary because this driver is actually wrong in which is
> asking for the host to power itself. This is not specified on USB-MIDI
> specification, neither makes any sense since this configuration is
> device specific.
>
> What is your suggestion to make it configurable? Maybe at compile-time?
> I really don't know what is the best solution if this is not something
> you like it.
well, you could use our configfs-based gadget interface. You don't
really need to use gmidi.ko at all. In fact, we wanna do away with any
static modules and rely only on configfs. If configfs doesn't let you
change what you want/need, then we can talk about adding support for
those.
bMaxPower and bmAttributes sound like good things to have configurable
over configfs but beware of what the USB specification says about them,
we cannot let users violate the spec by passing bogus values on these
fields.
cheers
--
balbi
Hi Balbi, how are you?
On 07/03/16 10:59, Felipe Balbi wrote:
>
> Hi,
>
> Felipe Ferreri Tonello <[email protected]> writes:
>>>>> "Felipe F. Tonello" <[email protected]> writes:
>>>>>> [ text/plain ]
>>>>>> This gadget uses a bmAttributes and MaxPower that requires the USB
>>>>> bus to be
>>>>>> powered from the host, which is not correct because this
>>>>> configuration is device
>>>>>> specific, not a USB-MIDI requirement.
>>>>>>
>>>>>> This patch adds two modules parameters, bmAttributes and MaxPower,
>>>>> that allows
>>>>>> the user to set those flags. The default values are what the gadget
>>>>> used to use
>>>>>> for backward compatibility.
>>>>>>
>>>>>> Signed-off-by: Felipe F. Tonello <[email protected]>
>>>>>> ---
>>>>>> drivers/usb/gadget/legacy/gmidi.c | 14 ++++++++++++--
>>>>>> 1 file changed, 12 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/gadget/legacy/gmidi.c
>>>>> b/drivers/usb/gadget/legacy/gmidi.c
>>>>>> index fc2ac150f5ff..0553435cc360 100644
>>>>>> --- a/drivers/usb/gadget/legacy/gmidi.c
>>>>>> +++ b/drivers/usb/gadget/legacy/gmidi.c
>>>>>> @@ -63,6 +63,14 @@ static unsigned int out_ports = 1;
>>>>>> module_param(out_ports, uint, S_IRUGO);
>>>>>> MODULE_PARM_DESC(out_ports, "Number of MIDI output ports");
>>>>>>
>>>>>> +static unsigned int bmAttributes = USB_CONFIG_ATT_ONE;
>>>>>> +module_param(bmAttributes, uint, S_IRUGO);
>>>>>> +MODULE_PARM_DESC(bmAttributes, "Configuration Descriptor's
>>>>> bmAttributes parameter");
>>>>>> +
>>>>>> +static unsigned int MaxPower = CONFIG_USB_GADGET_VBUS_DRAW;
>>>>>> +module_param(MaxPower, uint, S_IRUGO);
>>>>>> +MODULE_PARM_DESC(MaxPower, "Used to calculate Configuration
>>>>> Descriptor's bMaxPower parameter");
>>>>>
>>>>> you didn't run checkpatch, did you ? Also, are you sure you will need
>>>>> to
>>>>> change this by simply reloading the module ? I'm not convinced.
>>>>
>>>> Yes I always run checkpatch :)
>>>
>>> do you really ? Why do you have a 98-character line, then ?
>>>
>>>> What do you mean by reloading the module?
>>>
>>> modprobe g_midi MaxPower=4
>>> modprobe -r g_midi
>>> modprobe g_midi MaxPower=10
>>>
>>> I'm not convinced this is a valid use-case. Specially since you can just
>>> provide several configurations and let the host choose the one it suits
>>> it best.
>>
>> Ok, I will further test it.
>>
>>>
>>>>>> @@ -119,8 +127,8 @@ static struct usb_configuration midi_config = {
>>>>>> .label = "MIDI Gadget",
>>>>>> .bConfigurationValue = 1,
>>>>>> /* .iConfiguration = DYNAMIC */
>>>>>> - .bmAttributes = USB_CONFIG_ATT_ONE,
>>>>>
>>>>> nack, nackety nack nack nack :-)
>>>>>
>>>>> USB_CONFIG_ATT_ONE *must* always be set. With your module parameter you
>>>>> give users the oportunity to violate USB spec.
>>>>
>>>> You are right. It needs to check the value before it assigns to
>>>> bmAttributes.
>>>
>>> why check ? You can just unconditionally or USB_CONFIG_ATT_ONE. In any
>>> case, I'm not convinced this is necessary at all.
>>
>> Right.
>>
>> This is necessary because this driver is actually wrong in which is
>> asking for the host to power itself. This is not specified on USB-MIDI
>> specification, neither makes any sense since this configuration is
>> device specific.
>>
>> What is your suggestion to make it configurable? Maybe at compile-time?
>> I really don't know what is the best solution if this is not something
>> you like it.
>
> well, you could use our configfs-based gadget interface. You don't
> really need to use gmidi.ko at all. In fact, we wanna do away with any
> static modules and rely only on configfs. If configfs doesn't let you
> change what you want/need, then we can talk about adding support for
> those.
>
> bMaxPower and bmAttributes sound like good things to have configurable
> over configfs but beware of what the USB specification says about them,
> we cannot let users violate the spec by passing bogus values on these
> fields.
I agree that we should move to configfs, but the truth is that these
legacy devices are still useful. They just do one thing, mostly, but its
easy and simple to setup and use. So I think before we have some sort of
preset library of configfs-based gadget drivers, we still need these
modules.
Any suggestions on that?
Do you want to support what I am proposing for gmidi.ko or just ignore
it and move on to configfs?
Felipe
Hi,
Felipe Ferreri Tonello <[email protected]> writes:
>>>>> Since f_midi_transmit is called by both ALSA and USB frameworks, it
>>>> can
>>>>> potentially cause a race condition between both calls. This is bad
>>>> because the
>>>>> way f_midi_transmit is implemented can't handle concurrent calls.
>>>> This is due
>>>>> to the fact that the usb request fifo looks for the next element and
>>>> only if
>>>>> it has data to process it enqueues the request, otherwise re-uses it.
>>>> If both
>>>>> (ALSA and USB) frameworks calls this function at the same time, the
>>>>> kfifo_seek() will return the same usb_request, which will cause a
>>>> race
>>>>> condition.
>>>>>
>>>>> To solve this problem a syncronization mechanism is necessary. In
>>>> this case it
>>>>> is used a spinlock since f_midi_transmit is also called by
>>>> usb_request->complete
>>>>> callback in interrupt context.
>>>>>
>>>>> On benchmarks realized by me, spinlocks were more efficient then
>>>> scheduling
>>>>> the f_midi_transmit tasklet in process context and using a mutex
>>>>> to synchronize. Also it performs better then previous
>>>>> implementation
>>>> that
>>>>> allocated a usb_request for every new transmit made.
>>>>
>>>> behaves better in what way ? Also, previous implementation would not
>>>> suffer from this concurrency problem, right ?
>>>
>>> The spin lock is faster than allocating usb requests all the time,
>>> even if the udc uses da for it.
>>
>> did you measure ? Is the extra speed really necessary ? How did you
>> benchmark this ?
>
> Yes I did measure and it was not that significant. This is not about
> speed. There was a bug in that approach that I already explained on
you have very confusing statements. When I mentioned that previous code
wouldn't have the need for the spinlock you replied that spinlock was
faster.
When I asked you about benchmarks you reply saying it's not about the
speed.
Make up your mind dude. What are you trying to achieve ?
> that patch, which was approved and applied BTW.
patches can be reverted if we realise we're better off without
them. Don't get cocky, please.
> Any way, this spinlock should've been there since that patch but I
> couldn't really trigger this problem without a stress test.
which tells me you sent me patches without properly testing. How much
time did it take to trigger this ? How did you trigger this situation ?
> So, this patch fixes a bug in the current implementation.
fixes a regression introduced by you, true. I'm trying to figure out if
we're better off without the original patch; to make a good decision I
need to know if the extra "speed" we get from not allocating requests on
demand are really that important.
So, how much faster did you get and is that extra "speed" really
important ?
--
balbi
Hi,
Felipe Ferreri Tonello <[email protected]> writes:
>>>>>>> @@ -63,6 +63,14 @@ static unsigned int out_ports = 1;
>>>>>>> module_param(out_ports, uint, S_IRUGO);
>>>>>>> MODULE_PARM_DESC(out_ports, "Number of MIDI output ports");
>>>>>>>
>>>>>>> +static unsigned int bmAttributes = USB_CONFIG_ATT_ONE;
>>>>>>> +module_param(bmAttributes, uint, S_IRUGO);
>>>>>>> +MODULE_PARM_DESC(bmAttributes, "Configuration Descriptor's
>>>>>> bmAttributes parameter");
>>>>>>> +
>>>>>>> +static unsigned int MaxPower = CONFIG_USB_GADGET_VBUS_DRAW;
>>>>>>> +module_param(MaxPower, uint, S_IRUGO);
>>>>>>> +MODULE_PARM_DESC(MaxPower, "Used to calculate Configuration
>>>>>> Descriptor's bMaxPower parameter");
>>>>>>
>>>>>> you didn't run checkpatch, did you ? Also, are you sure you will need
>>>>>> to
>>>>>> change this by simply reloading the module ? I'm not convinced.
>>>>>
>>>>> Yes I always run checkpatch :)
>>>>
>>>> do you really ? Why do you have a 98-character line, then ?
btw, you didn't reply this ^^
>>>>>>> @@ -119,8 +127,8 @@ static struct usb_configuration midi_config = {
>>>>>>> .label = "MIDI Gadget",
>>>>>>> .bConfigurationValue = 1,
>>>>>>> /* .iConfiguration = DYNAMIC */
>>>>>>> - .bmAttributes = USB_CONFIG_ATT_ONE,
>>>>>>
>>>>>> nack, nackety nack nack nack :-)
>>>>>>
>>>>>> USB_CONFIG_ATT_ONE *must* always be set. With your module parameter you
>>>>>> give users the oportunity to violate USB spec.
>>>>>
>>>>> You are right. It needs to check the value before it assigns to
>>>>> bmAttributes.
>>>>
>>>> why check ? You can just unconditionally or USB_CONFIG_ATT_ONE. In any
>>>> case, I'm not convinced this is necessary at all.
>>>
>>> Right.
>>>
>>> This is necessary because this driver is actually wrong in which is
>>> asking for the host to power itself. This is not specified on USB-MIDI
>>> specification, neither makes any sense since this configuration is
>>> device specific.
>>>
>>> What is your suggestion to make it configurable? Maybe at compile-time?
>>> I really don't know what is the best solution if this is not something
>>> you like it.
>>
>> well, you could use our configfs-based gadget interface. You don't
>> really need to use gmidi.ko at all. In fact, we wanna do away with any
>> static modules and rely only on configfs. If configfs doesn't let you
>> change what you want/need, then we can talk about adding support for
>> those.
>>
>> bMaxPower and bmAttributes sound like good things to have configurable
>> over configfs but beware of what the USB specification says about them,
>> we cannot let users violate the spec by passing bogus values on these
>> fields.
>
> I agree that we should move to configfs, but the truth is that these
> legacy devices are still useful. They just do one thing, mostly, but
yes, they are useful as they are. They don't need to be changed to be
useful. Plus, you can have a gadget built with configfs that does only
one thing. And you can do that with a simple shell script.
> its easy and simple to setup and use. So I think before we have some
so is configfs.
> sort of preset library of configfs-based gadget drivers, we still need
> these modules.
there is already a library called libusbg.
> Any suggestions on that?
>
> Do you want to support what I am proposing for gmidi.ko or just ignore
> it and move on to configfs?
I prefer to not touch these gadget drivers if at all necessary. If you
fixing a bug, then sure we must fix bugs. But you're not fixing a bug
and, on top of that, you're adding regressions and violating the USB
spec. This shows that you're writing these patches without knowing
(and/or even caring about) the specification at all.
Here's some enlightening presentation you probably wanna watch:
https://www.youtube.com/watch?v=fMeH7wqOwXA
TL;DR : this project is large and you need to convince me we need your
code/patch.
--
balbi
Hi,
Felipe Ferreri Tonello <[email protected]> writes:
>>> On March 5, 2016 4:28:45 PM GMT+00:00, Michal Nazarewicz <[email protected]> wrote:
>>>>>> On Wed, Mar 02 2016, Felipe F. Tonello wrote:
>>>>>>> @@ -16,7 +16,7 @@
>>>>>>> * Copyright (C) 2006 Thumtronics Pty Ltd.
>>>>>>> * Ben Williamson <[email protected]>
>>>>>>> *
>>>>>>> - * Licensed under the GPL-2 or later.
>>>>>>> + * Licensed under the GPLv2.
>>>>
>>>>> On March 4, 2016 7:17:31 PM GMT+00:00, Michal Nazarewicz
>>>> <[email protected]> wrote:
>>>>>> Any particular reason to do that?
>>>>
>>>> On Fri, Mar 04 2016, Felipe Ferreri Tonello wrote:
>>>>> Because the kernel is v2 only and not later.
>>>>
>>>> Linux as a whole is GPLv2 only, but that doesn’t necessarily mean that
>>>> parts of it cannot be dual licensed (or GPLv2+). It’s safer to leave
>>>> copyright noticed clear unless you explicitly want your contribution be
>>>> GPLv2 only which brings the whole file GPLv2 only.
>>>>
>>>>> I just tried to make this driver more consistent with the coding
>>>> style
>>>>> used across the kernel. That's it.
>>>>
>>>> Column alignment of field names or RHS of assignment operators is quite
>>>> inconsistent already within drivers/usb/gadget/ which is why I’m
>>>> concerned whether this is really helping.
>>>>
>>>> Anyway, I actually don’t care much, just adding my two rappen.
>>>
>>> Right, I am ok with Balbi completely ignoring this patch. But I prefer
>>> to have at least this driver consistent than nothing. Of course I'll
>>> remove the license change I made.
>>
>> consistent in what way ?
>
> Source-code.
>
> The goal of this patch is to update this driver coding style to promote
> consistency, readability, and maintainability based on the Linux coding
> style.
>
> If this patch does not achieving that or if that is not necessary, than
> just ignore this patch.
yeah, I don't think that's what you're doing here.
--
balbi
On 03/08/2016 08:43 AM, Felipe Balbi wrote:
(...)
>>>> This is necessary because this driver is actually wrong in which is
>>>> asking for the host to power itself. This is not specified on USB-MIDI
>>>> specification, neither makes any sense since this configuration is
>>>> device specific.
>>>>
>>>> What is your suggestion to make it configurable? Maybe at compile-time?
>>>> I really don't know what is the best solution if this is not something
>>>> you like it.
>>>
>>> well, you could use our configfs-based gadget interface. You don't
>>> really need to use gmidi.ko at all. In fact, we wanna do away with any
>>> static modules and rely only on configfs. If configfs doesn't let you
>>> change what you want/need, then we can talk about adding support for
>>> those.
>>>
>>> bMaxPower and bmAttributes sound like good things to have configurable
>>> over configfs but beware of what the USB specification says about them,
>>> we cannot let users violate the spec by passing bogus values on these
>>> fields.
>>
>> I agree that we should move to configfs, but the truth is that these
>> legacy devices are still useful. They just do one thing, mostly, but
>
> yes, they are useful as they are. They don't need to be changed to be
> useful. Plus, you can have a gadget built with configfs that does only
> one thing. And you can do that with a simple shell script.
>
>> its easy and simple to setup and use. So I think before we have some
>
> so is configfs.
>
>> sort of preset library of configfs-based gadget drivers, we still need
>> these modules.
>
> there is already a library called libusbg.
As libusbg itself is a little bit dead there is a fork called
libusbgx[1] and it is still active;)
It already has support for f_midi so it is ready to use.
Footnotes:
1 - https://github.com/libusbgx/libusbgx
Cheers,
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
Hi,
Krzysztof Opasiak <[email protected]> writes:
> [ text/plain ]
>
>
> On 03/08/2016 08:43 AM, Felipe Balbi wrote:
> (...)
>
>>>>> This is necessary because this driver is actually wrong in which is
>>>>> asking for the host to power itself. This is not specified on USB-MIDI
>>>>> specification, neither makes any sense since this configuration is
>>>>> device specific.
>>>>>
>>>>> What is your suggestion to make it configurable? Maybe at compile-time?
>>>>> I really don't know what is the best solution if this is not something
>>>>> you like it.
>>>>
>>>> well, you could use our configfs-based gadget interface. You don't
>>>> really need to use gmidi.ko at all. In fact, we wanna do away with any
>>>> static modules and rely only on configfs. If configfs doesn't let you
>>>> change what you want/need, then we can talk about adding support for
>>>> those.
>>>>
>>>> bMaxPower and bmAttributes sound like good things to have configurable
>>>> over configfs but beware of what the USB specification says about them,
>>>> we cannot let users violate the spec by passing bogus values on these
>>>> fields.
>>>
>>> I agree that we should move to configfs, but the truth is that these
>>> legacy devices are still useful. They just do one thing, mostly, but
>>
>> yes, they are useful as they are. They don't need to be changed to be
>> useful. Plus, you can have a gadget built with configfs that does only
>> one thing. And you can do that with a simple shell script.
>>
>>> its easy and simple to setup and use. So I think before we have some
>>
>> so is configfs.
>>
>>> sort of preset library of configfs-based gadget drivers, we still need
>>> these modules.
>>
>> there is already a library called libusbg.
>
> As libusbg itself is a little bit dead there is a fork called
> libusbgx[1] and it is still active;)
>
> It already has support for f_midi so it is ready to use.
heh, seems like usb libraries tend to get forked with an 'x' appended to
their name. But thanks for the note.
--
balbi
Hi Balbi,
On 08/03/16 07:37, Felipe Balbi wrote:
>
> Hi,
>
> Felipe Ferreri Tonello <[email protected]> writes:
>>>>>> Since f_midi_transmit is called by both ALSA and USB frameworks, it
>>>>> can
>>>>>> potentially cause a race condition between both calls. This is bad
>>>>> because the
>>>>>> way f_midi_transmit is implemented can't handle concurrent calls.
>>>>> This is due
>>>>>> to the fact that the usb request fifo looks for the next element and
>>>>> only if
>>>>>> it has data to process it enqueues the request, otherwise re-uses it.
>>>>> If both
>>>>>> (ALSA and USB) frameworks calls this function at the same time, the
>>>>>> kfifo_seek() will return the same usb_request, which will cause a
>>>>> race
>>>>>> condition.
>>>>>>
>>>>>> To solve this problem a syncronization mechanism is necessary. In
>>>>> this case it
>>>>>> is used a spinlock since f_midi_transmit is also called by
>>>>> usb_request->complete
>>>>>> callback in interrupt context.
>>>>>>
>>>>>> On benchmarks realized by me, spinlocks were more efficient then
>>>>> scheduling
>>>>>> the f_midi_transmit tasklet in process context and using a mutex
>>>>>> to synchronize. Also it performs better then previous
>>>>>> implementation
>>>>> that
>>>>>> allocated a usb_request for every new transmit made.
>>>>>
>>>>> behaves better in what way ? Also, previous implementation would not
>>>>> suffer from this concurrency problem, right ?
>>>>
>>>> The spin lock is faster than allocating usb requests all the time,
>>>> even if the udc uses da for it.
>>>
>>> did you measure ? Is the extra speed really necessary ? How did you
>>> benchmark this ?
>>
>> Yes I did measure and it was not that significant. This is not about
>> speed. There was a bug in that approach that I already explained on
>
> you have very confusing statements. When I mentioned that previous code
> wouldn't have the need for the spinlock you replied that spinlock was
> faster.
>
> When I asked you about benchmarks you reply saying it's not about the
> speed.
>
> Make up your mind dude. What are you trying to achieve ?
>
>> that patch, which was approved and applied BTW.
>
> patches can be reverted if we realise we're better off without
> them. Don't get cocky, please.
Yes am I aware of that, but I honestly think that is the wrong way of
dealing with this.
?? I don't get why am I giving this impression.
>
>> Any way, this spinlock should've been there since that patch but I
>> couldn't really trigger this problem without a stress test.
>
> which tells me you sent me patches without properly testing. How much
> time did it take to trigger this ? How did you trigger this situation ?
No, that is no true. The implementation I sent is working properly for
any real world usage.
The stress test I made to break the current implementation is *not* a
real use-case. I made it in order to push as far as possible how fast
the driver can *reliably* handle while sending and reading data. Then I
noticed the bug.
So, to answer your question. To trigger this bug is not a matter of
time. The following needs to happen:
1. Device send MIDI message that is *bigger* than the usb request
length. (just this by itself is really unlikely to happen in real world
usage)
2. Host send a MIDI message back *exactly* at the same time as the
device is processing the second part of the usb request from the same
message.
I couldn't trigger this in all the tests we've made. I just triggered
when I was sending huge messages back and forth (device <-> host) as
mentioned.
In fact, we have thousands of devices out there without this patch (but
with my previous patch that introduced this bug).
I am not trying to say it wasn't a mistake. That patch unfortunately
introduces this bug, but it has real improvements over the previous
implementation. AFAIR the improvements are:
* Fixes a bug that was causing the DMA buffer to fill it up causing a
kernel panic.
* Pre allocate IN usb requests so there is no allocation overhead while
sending data (same behavior already existed for the OUT endpoint). This
ensure that the DMA memory is not misused affecting the rest of the system.
* It doesn't crash if the host doesn't send an ACK after IN data
packets and we have reached the limit of available memory. Also, this is
useful because it causes the ALSA layer to timeout, which is the correct
userspace behavior.
* Continuous to send data to the correct Jack (associated to each ALSA
substream) if that was interrupted somehow, for instance by the size
limit of a usb request.
>
>> So, this patch fixes a bug in the current implementation.
>
> fixes a regression introduced by you, true. I'm trying to figure out if
> we're better off without the original patch; to make a good decision I
> need to know if the extra "speed" we get from not allocating requests on
> demand are really that important.
>
> So, how much faster did you get and is that extra "speed" really
> important ?
The speed is not relevant at all in this case. It was not the goal of
the patch, but I mentioned because it is obvious that with no memory
allocation there will be an increase of speed that the code is executed.
I did measure the speed improvements at that time, it was real but not
relevant. I don't think we should be discussing this anyway.
--
Felipe
Hi Balbi,
On 08/03/16 07:43, Felipe Balbi wrote:
>
> Hi,
>
> Felipe Ferreri Tonello <[email protected]> writes:
>>>>>>>> @@ -63,6 +63,14 @@ static unsigned int out_ports = 1;
>>>>>>>> module_param(out_ports, uint, S_IRUGO);
>>>>>>>> MODULE_PARM_DESC(out_ports, "Number of MIDI output ports");
>>>>>>>>
>>>>>>>> +static unsigned int bmAttributes = USB_CONFIG_ATT_ONE;
>>>>>>>> +module_param(bmAttributes, uint, S_IRUGO);
>>>>>>>> +MODULE_PARM_DESC(bmAttributes, "Configuration Descriptor's
>>>>>>> bmAttributes parameter");
>>>>>>>> +
>>>>>>>> +static unsigned int MaxPower = CONFIG_USB_GADGET_VBUS_DRAW;
>>>>>>>> +module_param(MaxPower, uint, S_IRUGO);
>>>>>>>> +MODULE_PARM_DESC(MaxPower, "Used to calculate Configuration
>>>>>>> Descriptor's bMaxPower parameter");
>>>>>>>
>>>>>>> you didn't run checkpatch, did you ? Also, are you sure you will need
>>>>>>> to
>>>>>>> change this by simply reloading the module ? I'm not convinced.
>>>>>>
>>>>>> Yes I always run checkpatch :)
>>>>>
>>>>> do you really ? Why do you have a 98-character line, then ?
>
> btw, you didn't reply this ^^
>
>>>>>>>> @@ -119,8 +127,8 @@ static struct usb_configuration midi_config = {
>>>>>>>> .label = "MIDI Gadget",
>>>>>>>> .bConfigurationValue = 1,
>>>>>>>> /* .iConfiguration = DYNAMIC */
>>>>>>>> - .bmAttributes = USB_CONFIG_ATT_ONE,
>>>>>>>
>>>>>>> nack, nackety nack nack nack :-)
>>>>>>>
>>>>>>> USB_CONFIG_ATT_ONE *must* always be set. With your module parameter you
>>>>>>> give users the oportunity to violate USB spec.
>>>>>>
>>>>>> You are right. It needs to check the value before it assigns to
>>>>>> bmAttributes.
>>>>>
>>>>> why check ? You can just unconditionally or USB_CONFIG_ATT_ONE. In any
>>>>> case, I'm not convinced this is necessary at all.
>>>>
>>>> Right.
>>>>
>>>> This is necessary because this driver is actually wrong in which is
>>>> asking for the host to power itself. This is not specified on USB-MIDI
>>>> specification, neither makes any sense since this configuration is
>>>> device specific.
>>>>
>>>> What is your suggestion to make it configurable? Maybe at compile-time?
>>>> I really don't know what is the best solution if this is not something
>>>> you like it.
>>>
>>> well, you could use our configfs-based gadget interface. You don't
>>> really need to use gmidi.ko at all. In fact, we wanna do away with any
>>> static modules and rely only on configfs. If configfs doesn't let you
>>> change what you want/need, then we can talk about adding support for
>>> those.
>>>
>>> bMaxPower and bmAttributes sound like good things to have configurable
>>> over configfs but beware of what the USB specification says about them,
>>> we cannot let users violate the spec by passing bogus values on these
>>> fields.
>>
>> I agree that we should move to configfs, but the truth is that these
>> legacy devices are still useful. They just do one thing, mostly, but
>
> yes, they are useful as they are. They don't need to be changed to be
> useful. Plus, you can have a gadget built with configfs that does only
> one thing. And you can do that with a simple shell script.
>
>> its easy and simple to setup and use. So I think before we have some
>
> so is configfs.
>
>> sort of preset library of configfs-based gadget drivers, we still need
>> these modules.
>
> there is already a library called libusbg.
By preset library I meant scripts or little programs that implement the
legacy drivers we have.
>
>> Any suggestions on that?
>>
>> Do you want to support what I am proposing for gmidi.ko or just ignore
>> it and move on to configfs?
>
> I prefer to not touch these gadget drivers if at all necessary. If you
> fixing a bug, then sure we must fix bugs. But you're not fixing a bug
> and, on top of that, you're adding regressions and violating the USB
> spec. This shows that you're writing these patches without knowing
> (and/or even caring about) the specification at all.
Yes, I see your point. My mistake was to not to enforce the first bit to
be set enabling the user to break the USB spec. I didn't think of that
scenario. And that's why it's always useful to have kernel maintainers
and others to provide such insights. :)
Anyway, I see that this patch is not useful even if corrected.
>
> Here's some enlightening presentation you probably wanna watch:
>
> https://www.youtube.com/watch?v=fMeH7wqOwXA
>
> TL;DR : this project is large and you need to convince me we need your
> code/patch.
>
Felipe
Hi,
Felipe Ferreri Tonello <[email protected]> writes:
>>>>>>> Since f_midi_transmit is called by both ALSA and USB frameworks, it
>>>>>> can
>>>>>>> potentially cause a race condition between both calls. This is bad
>>>>>> because the
>>>>>>> way f_midi_transmit is implemented can't handle concurrent calls.
>>>>>> This is due
>>>>>>> to the fact that the usb request fifo looks for the next element and
>>>>>> only if
>>>>>>> it has data to process it enqueues the request, otherwise re-uses it.
>>>>>> If both
>>>>>>> (ALSA and USB) frameworks calls this function at the same time, the
>>>>>>> kfifo_seek() will return the same usb_request, which will cause a
>>>>>> race
>>>>>>> condition.
>>>>>>>
>>>>>>> To solve this problem a syncronization mechanism is necessary. In
>>>>>> this case it
>>>>>>> is used a spinlock since f_midi_transmit is also called by
>>>>>> usb_request->complete
>>>>>>> callback in interrupt context.
>>>>>>>
>>>>>>> On benchmarks realized by me, spinlocks were more efficient then
>>>>>> scheduling
>>>>>>> the f_midi_transmit tasklet in process context and using a mutex
>>>>>>> to synchronize. Also it performs better then previous
>>>>>>> implementation
>>>>>> that
>>>>>>> allocated a usb_request for every new transmit made.
>>>>>>
>>>>>> behaves better in what way ? Also, previous implementation would not
>>>>>> suffer from this concurrency problem, right ?
>>>>>
>>>>> The spin lock is faster than allocating usb requests all the time,
>>>>> even if the udc uses da for it.
>>>>
>>>> did you measure ? Is the extra speed really necessary ? How did you
>>>> benchmark this ?
>>>
>>> Yes I did measure and it was not that significant. This is not about
>>> speed. There was a bug in that approach that I already explained on
>>
>> you have very confusing statements. When I mentioned that previous code
>> wouldn't have the need for the spinlock you replied that spinlock was
>> faster.
>>
>> When I asked you about benchmarks you reply saying it's not about the
>> speed.
>>
>> Make up your mind dude. What are you trying to achieve ?
>>
>>> that patch, which was approved and applied BTW.
>>
>> patches can be reverted if we realise we're better off without
>> them. Don't get cocky, please.
>
> Yes am I aware of that, but I honestly think that is the wrong way of
> dealing with this.
>
> ?? I don't get why am I giving this impression.
re-read your emails. The gist goes like this:
. Send patch
. Got comments
. Well, whatever, you can just ignore if you don't agree
>>> Any way, this spinlock should've been there since that patch but I
>>> couldn't really trigger this problem without a stress test.
>>
>> which tells me you sent me patches without properly testing. How much
>> time did it take to trigger this ? How did you trigger this situation ?
>
> No, that is no true. The implementation I sent is working properly for
> any real world usage.
>
> The stress test I made to break the current implementation is *not* a
> real use-case. I made it in order to push as far as possible how fast
> the driver can *reliably* handle while sending and reading data. Then I
> noticed the bug.
>
> So, to answer your question. To trigger this bug is not a matter of
> time. The following needs to happen:
> 1. Device send MIDI message that is *bigger* than the usb request
> length. (just this by itself is really unlikely to happen in real world
> usage)
I wouldn't say it's unlikely. You just cannot trust the other side of
the wire. We've seen e.g. Xbox 360's SCSI layer sending messages of the
wrong size and we worked around them in g_mass_storage.
Broken implementations are a real thing ;-)
> 2. Host send a MIDI message back *exactly* at the same time as the
> device is processing the second part of the usb request from the same
> message.
also not that unlikely to happen ;-) You can't assume the host will only
shift tokens on the wire at the time you're expecting it to.
> I couldn't trigger this in all the tests we've made. I just triggered
> when I was sending huge messages back and forth (device <-> host) as
> mentioned.
fair enough.
> In fact, we have thousands of devices out there without this patch (but
> with my previous patch that introduced this bug).
that's thousands of devices waiting to have a problem, right ? :-)
> I am not trying to say it wasn't a mistake. That patch unfortunately
> introduces this bug, but it has real improvements over the previous
> implementation. AFAIR the improvements are:
> * Fixes a bug that was causing the DMA buffer to fill it up causing a
> kernel panic.
this is a good point. Had forgotten about that detail. Thanks
> * Pre allocate IN usb requests so there is no allocation overhead while
> sending data (same behavior already existed for the OUT endpoint). This
> ensure that the DMA memory is not misused affecting the rest of the
> system.
also, arguably, a good idea. Recycling requests is a lot nicer and it's
what most gadget drivers do.
> * It doesn't crash if the host doesn't send an ACK after IN data
> packets and we have reached the limit of available memory. Also, this is
> useful because it causes the ALSA layer to timeout, which is the correct
> userspace behavior.
right
> * Continuous to send data to the correct Jack (associated to each ALSA
> substream) if that was interrupted somehow, for instance by the size
> limit of a usb request.
ok.
>>> So, this patch fixes a bug in the current implementation.
>>
>> fixes a regression introduced by you, true. I'm trying to figure out if
>> we're better off without the original patch; to make a good decision I
>> need to know if the extra "speed" we get from not allocating requests on
>> demand are really that important.
>>
>> So, how much faster did you get and is that extra "speed" really
>> important ?
>
> The speed is not relevant at all in this case. It was not the goal of
> the patch, but I mentioned because it is obvious that with no memory
> allocation there will be an increase of speed that the code is executed.
>
> I did measure the speed improvements at that time, it was real but not
> relevant. I don't think we should be discussing this anyway.
fair enough. This was probably the first email from you which gave me
some peace of mind that you know what you're doing with this fix. Keep
in mind that we all receive hundreds of emails a day and it's difficult
to track things over time.
It's also a big PITA when someone sends fixes and cleanups on the same
series and/or with dependencies between them. The correct way is to send
*only* fixes first. They should be minimal patches that *only* fix the
problem. If the code looks messy or doesn't follow the coding style,
that's something you do on a completely separate fix and, usually, from
a clean topic branch starting at a tag from Linus (exceptions may arise,
of course).
So anyway, to finally finish this up. Can you send JUST the bare minimum
fix necessary to avoid the regression ? Also, add a proper Fixes: foobar
line on commit log (see commit e18b7975c885bc3a938b9a76daf32957ea0235fa
for an example).
Then we can get that merged. Keep in mind that you might have to Cc
stable (see same commit listed above).
After this is sorted out, then let's see how we can help you move your
product to libusbgx and check if there's anything missing in configfs
to cope with your use-case.
ps: can you point me to your devices shipping with f_midi ? Which
architecture are they using ? Which USB Peripheral Controller ? This
might be a good addition to my test farm depending on your answers above
:-p
cheers
--
balbi
Hi,
Felipe Ferreri Tonello <[email protected]> writes:
>>> its easy and simple to setup and use. So I think before we have some
>>
>> so is configfs.
>>
>>> sort of preset library of configfs-based gadget drivers, we still need
>>> these modules.
>>
>> there is already a library called libusbg.
>
> By preset library I meant scripts or little programs that implement the
> legacy drivers we have.
like this ?
https://github.com/libusbgx/libusbgx/blob/master/examples/gadget-midi.c
>>> Any suggestions on that?
>>>
>>> Do you want to support what I am proposing for gmidi.ko or just ignore
>>> it and move on to configfs?
>>
>> I prefer to not touch these gadget drivers if at all necessary. If you
>> fixing a bug, then sure we must fix bugs. But you're not fixing a bug
>> and, on top of that, you're adding regressions and violating the USB
>> spec. This shows that you're writing these patches without knowing
>> (and/or even caring about) the specification at all.
>
> Yes, I see your point. My mistake was to not to enforce the first bit to
> be set enabling the user to break the USB spec. I didn't think of that
right, that was the problem.
> scenario. And that's why it's always useful to have kernel maintainers
> and others to provide such insights. :)
yeah, no problem ;-)
--
balbi
On 03/08/2016 02:54 PM, Felipe Ferreri Tonello wrote:
(...)
>>
>>> sort of preset library of configfs-based gadget drivers, we still need
>>> these modules.
>>
>> there is already a library called libusbg.
>
> By preset library I meant scripts or little programs that implement the
> legacy drivers we have.
>
libusbgx implements an idea of gadget schemes[1]. That's simple
configuration files using libconfig syntax. I don't see any problems to
use it to create legacy gadget equivalents. Then you could simply load
it using usbg_import_gadget() in C code or gt[2] load from shell.
Footnotes:
1 - https://github.com/libusbgx/libusbgx/blob/master/doc/gadget_schemes.txt
2 - https://github.com/kopasiak/gt
Cheers,
--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
Hi,
Krzysztof Opasiak <[email protected]> writes:
> [ text/plain ]
>
>
> On 03/08/2016 02:54 PM, Felipe Ferreri Tonello wrote:
> (...)
>
>>>
>>>> sort of preset library of configfs-based gadget drivers, we still need
>>>> these modules.
>>>
>>> there is already a library called libusbg.
>>
>> By preset library I meant scripts or little programs that implement the
>> legacy drivers we have.
>>
>
> libusbgx implements an idea of gadget schemes[1]. That's simple
> configuration files using libconfig syntax. I don't see any problems to
> use it to create legacy gadget equivalents. Then you could simply load
> it using usbg_import_gadget() in C code or gt[2] load from shell.
cool, bmAttributes and bMaxPower are already there. Nice.
--
balbi
Hi,
On 08/03/16 14:20, Felipe Balbi wrote:
>
> Hi,
>
> Krzysztof Opasiak <[email protected]> writes:
>> [ text/plain ]
>>
>>
>> On 03/08/2016 02:54 PM, Felipe Ferreri Tonello wrote:
>> (...)
>>
>>>>
>>>>> sort of preset library of configfs-based gadget drivers, we still need
>>>>> these modules.
>>>>
>>>> there is already a library called libusbg.
>>>
>>> By preset library I meant scripts or little programs that implement the
>>> legacy drivers we have.
>>>
>>
>> libusbgx implements an idea of gadget schemes[1]. That's simple
>> configuration files using libconfig syntax. I don't see any problems to
>> use it to create legacy gadget equivalents. Then you could simply load
>> it using usbg_import_gadget() in C code or gt[2] load from shell.
>
> cool, bmAttributes and bMaxPower are already there. Nice.
>
Yes! It is pretty awesome.
--
Felipe
Hi Balbi,
On 08/03/16 14:01, Felipe Balbi wrote:
>
> Hi,
>
> Felipe Ferreri Tonello <[email protected]> writes:
>>>>>>>> Since f_midi_transmit is called by both ALSA and USB frameworks, it
>>>>>>> can
>>>>>>>> potentially cause a race condition between both calls. This is bad
>>>>>>> because the
>>>>>>>> way f_midi_transmit is implemented can't handle concurrent calls.
>>>>>>> This is due
>>>>>>>> to the fact that the usb request fifo looks for the next element and
>>>>>>> only if
>>>>>>>> it has data to process it enqueues the request, otherwise re-uses it.
>>>>>>> If both
>>>>>>>> (ALSA and USB) frameworks calls this function at the same time, the
>>>>>>>> kfifo_seek() will return the same usb_request, which will cause a
>>>>>>> race
>>>>>>>> condition.
>>>>>>>>
>>>>>>>> To solve this problem a syncronization mechanism is necessary. In
>>>>>>> this case it
>>>>>>>> is used a spinlock since f_midi_transmit is also called by
>>>>>>> usb_request->complete
>>>>>>>> callback in interrupt context.
>>>>>>>>
>>>>>>>> On benchmarks realized by me, spinlocks were more efficient then
>>>>>>> scheduling
>>>>>>>> the f_midi_transmit tasklet in process context and using a mutex
>>>>>>>> to synchronize. Also it performs better then previous
>>>>>>>> implementation
>>>>>>> that
>>>>>>>> allocated a usb_request for every new transmit made.
>>>>>>>
>>>>>>> behaves better in what way ? Also, previous implementation would not
>>>>>>> suffer from this concurrency problem, right ?
>>>>>>
>>>>>> The spin lock is faster than allocating usb requests all the time,
>>>>>> even if the udc uses da for it.
>>>>>
>>>>> did you measure ? Is the extra speed really necessary ? How did you
>>>>> benchmark this ?
>>>>
>>>> Yes I did measure and it was not that significant. This is not about
>>>> speed. There was a bug in that approach that I already explained on
>>>
>>> you have very confusing statements. When I mentioned that previous code
>>> wouldn't have the need for the spinlock you replied that spinlock was
>>> faster.
>>>
>>> When I asked you about benchmarks you reply saying it's not about the
>>> speed.
>>>
>>> Make up your mind dude. What are you trying to achieve ?
>>>
>>>> that patch, which was approved and applied BTW.
>>>
>>> patches can be reverted if we realise we're better off without
>>> them. Don't get cocky, please.
>>
>> Yes am I aware of that, but I honestly think that is the wrong way of
>> dealing with this.
>>
>> ?? I don't get why am I giving this impression.
>
> re-read your emails. The gist goes like this:
>
> . Send patch
> . Got comments
> . Well, whatever, you can just ignore if you don't agree
This is one of the problems with email. It can give the wrong impression
and feelings. :)
That was not what I meant at all. I mean that for real, not in a
childish manner. I'm sorry if I gave you that impression.
>
>>>> Any way, this spinlock should've been there since that patch but I
>>>> couldn't really trigger this problem without a stress test.
>>>
>>> which tells me you sent me patches without properly testing. How much
>>> time did it take to trigger this ? How did you trigger this situation ?
>>
>> No, that is no true. The implementation I sent is working properly for
>> any real world usage.
>>
>> The stress test I made to break the current implementation is *not* a
>> real use-case. I made it in order to push as far as possible how fast
>> the driver can *reliably* handle while sending and reading data. Then I
>> noticed the bug.
>>
>> So, to answer your question. To trigger this bug is not a matter of
>> time. The following needs to happen:
>> 1. Device send MIDI message that is *bigger* than the usb request
>> length. (just this by itself is really unlikely to happen in real world
>> usage)
>
> I wouldn't say it's unlikely. You just cannot trust the other side of
> the wire. We've seen e.g. Xbox 360's SCSI layer sending messages of the
> wrong size and we worked around them in g_mass_storage.
>
> Broken implementations are a real thing ;-)
Fair enough. And that's why I am pushing this fix. :)
>
>> 2. Host send a MIDI message back *exactly* at the same time as the
>> device is processing the second part of the usb request from the same
>> message.
>
> also not that unlikely to happen ;-) You can't assume the host will only
> shift tokens on the wire at the time you're expecting it to.
>
>> I couldn't trigger this in all the tests we've made. I just triggered
>> when I was sending huge messages back and forth (device <-> host) as
>> mentioned.
>
> fair enough.
>
>> In fact, we have thousands of devices out there without this patch (but
>> with my previous patch that introduced this bug).
>
> that's thousands of devices waiting to have a problem, right ? :-)
:X
>
>> I am not trying to say it wasn't a mistake. That patch unfortunately
>> introduces this bug, but it has real improvements over the previous
>> implementation. AFAIR the improvements are:
>> * Fixes a bug that was causing the DMA buffer to fill it up causing a
>> kernel panic.
>
> this is a good point. Had forgotten about that detail. Thanks
>
>> * Pre allocate IN usb requests so there is no allocation overhead while
>> sending data (same behavior already existed for the OUT endpoint). This
>> ensure that the DMA memory is not misused affecting the rest of the
>> system.
>
> also, arguably, a good idea. Recycling requests is a lot nicer and it's
> what most gadget drivers do.
>
>> * It doesn't crash if the host doesn't send an ACK after IN data
>> packets and we have reached the limit of available memory. Also, this is
>> useful because it causes the ALSA layer to timeout, which is the correct
>> userspace behavior.
>
> right
>
>> * Continuous to send data to the correct Jack (associated to each ALSA
>> substream) if that was interrupted somehow, for instance by the size
>> limit of a usb request.
>
> ok.
>
>>>> So, this patch fixes a bug in the current implementation.
>>>
>>> fixes a regression introduced by you, true. I'm trying to figure out if
>>> we're better off without the original patch; to make a good decision I
>>> need to know if the extra "speed" we get from not allocating requests on
>>> demand are really that important.
>>>
>>> So, how much faster did you get and is that extra "speed" really
>>> important ?
>>
>> The speed is not relevant at all in this case. It was not the goal of
>> the patch, but I mentioned because it is obvious that with no memory
>> allocation there will be an increase of speed that the code is executed.
>>
>> I did measure the speed improvements at that time, it was real but not
>> relevant. I don't think we should be discussing this anyway.
>
> fair enough. This was probably the first email from you which gave me
> some peace of mind that you know what you're doing with this fix. Keep
> in mind that we all receive hundreds of emails a day and it's difficult
> to track things over time.
True. I will try to keep this always in mind.
>
> It's also a big PITA when someone sends fixes and cleanups on the same
> series and/or with dependencies between them. The correct way is to send
> *only* fixes first. They should be minimal patches that *only* fix the
> problem. If the code looks messy or doesn't follow the coding style,
> that's something you do on a completely separate fix and, usually, from
> a clean topic branch starting at a tag from Linus (exceptions may arise,
> of course).
Got it.
>
> So anyway, to finally finish this up. Can you send JUST the bare minimum
> fix necessary to avoid the regression ? Also, add a proper Fixes: foobar
> line on commit log (see commit e18b7975c885bc3a938b9a76daf32957ea0235fa
> for an example).
>
> Then we can get that merged. Keep in mind that you might have to Cc
> stable (see same commit listed above).
Ok.
I will send the state-machine refactor as another patch in another topic
then.
>
> After this is sorted out, then let's see how we can help you move your
> product to libusbgx and check if there's anything missing in configfs
> to cope with your use-case.
That will be great, thanks! I will keep the list posted.
>
> ps: can you point me to your devices shipping with f_midi ? Which
> architecture are they using ? Which USB Peripheral Controller ? This
> might be a good addition to my test farm depending on your answers above
> :-p
Seaboard GRAND[1]. Freescale's i.MX 6 running an ARM A9. The controller
is Chip Idea.
[1] https://www.roli.com/products/seaboard-grand
--
Felipe
Hi,
Felipe Ferreri Tonello <[email protected]> writes:
>> ps: can you point me to your devices shipping with f_midi ? Which
>> architecture are they using ? Which USB Peripheral Controller ? This
>> might be a good addition to my test farm depending on your answers above
>> :-p
>
> Seaboard GRAND[1]. Freescale's i.MX 6 running an ARM A9. The controller
> is Chip Idea.
>
> [1] https://www.roli.com/products/seaboard-grand
nice-looking product. But probably above my
"yet-another-device-for-some-hacking-and-testing" budget. :-p
--
balbi