Re: [PATCH v4 1/7] usb: gadget: f_midi: Transmit data only when IN ep is enabled

[Felipe Tonello]

Hi Robert,

On Mon, Oct 26, 2015 at 10:13 PM, Robert Baldyga
 wrote:
> Hi Felipe,
>
> On 10/26/2015 05:55 PM, Felipe F. Tonello wrote:
>> This makes sure f_midi doesn't try to enqueue data when the IN endpoint is
>> disabled, ie, USB cable is disconnected.
>>
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>  drivers/usb/gadget/function/f_midi.c | 7 ++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c 
>> b/drivers/usb/gadget/function/f_midi.c
>> index edb84ca..e08f365 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -87,6 +87,7 @@ struct f_midi {
>>   int index;
>>   char *id;
>>   unsigned int buflen, qlen;
>> + bool in_ep_enabled;
>
> It's not necessary, you can use ep->enabled flag instead.

There is no such flag in usb_ep struct[1].

[1] http://lxr.free-electrons.com/source/include/linux/usb/gadget.h#L170

>
>>  };
>>
>>  static inline struct f_midi *func_to_midi(struct usb_function *f)
>> @@ -332,6 +333,7 @@ static int f_midi_set_alt(struct usb_function *f, 
>> unsigned intf, unsigned alt)
>>   err = f_midi_start_ep(midi, f, midi->in_ep);
>>   if (err)
>>   return err;
>> + midi->in_ep_enabled = true;
>>
>>   err = f_midi_start_ep(midi, f, midi->out_ep);
>>   if (err)
>> @@ -387,6 +389,8 @@ static void f_midi_disable(struct usb_function *f)
>>*/
>>   usb_ep_disable(midi->in_ep);
>>   usb_ep_disable(midi->out_ep);
>> +
>> + midi->in_ep_enabled = false;
>>  }
>>
>>  static int f_midi_snd_free(struct snd_device *device)
>> @@ -543,7 +547,7 @@ static void f_midi_transmit(struct f_midi *midi, struct 
>> usb_request *req)
>>   }
>>   }
>>
>> - if (req->length > 0) {
>> + if (req->length > 0 && midi->in_ep_enabled) {
>
> You should rather test it at the beginning of this function. Or, even
> better, when tasklet is scheduled, because tasklet is the only way this
> function can be called when endpoints are disabled.

Not in this case, because this function needs to consume the triggered
data from ALSA, otherwise a timeout will happen (which is worse).
Of course this is not the best solution, but it is an incremental improvement.

Patch 7 has the proper solution, which checks this flag at the
beginning of the function as expected. Also, it is more optimal
because it drops all substreams buffers, instead of copying it.

>
>>   int err;
>>
>>   err = usb_ep_queue(ep, req, GFP_ATOMIC);
>> @@ -1158,6 +1162,7 @@ static struct usb_function *f_midi_alloc(struct 
>> usb_function_instance *fi)
>>   midi->index = opts->index;
>>   midi->buflen = opts->buflen;
>>   midi->qlen = opts->qlen;
>> + midi->in_ep_enabled = false;
>>   ++opts->refcnt;
>>   mutex_unlock(>lock);
>>
>>
>

Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 4/7] usb: gadget: f_midi: fix leak on failed to enqueue out requests

[Felipe Tonello]

Hi Robert,

On Mon, Oct 26, 2015 at 10:23 PM, Robert Baldyga
 wrote:
> On 10/26/2015 05:55 PM, Felipe F. Tonello wrote:
>> This patch fixes a memory leak that occurs when an endpoint fails to enqueue
>> the request. If that happens the complete function will never be called, thus
>> never freeing the request.
>>
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>  drivers/usb/gadget/function/f_midi.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c 
>> b/drivers/usb/gadget/function/f_midi.c
>> index 4c01c8a..0e9cdeb 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -344,6 +344,7 @@ static int f_midi_set_alt(struct usb_function *f, 
>> unsigned intf, unsigned alt)
>>   if (err) {
>>   ERROR(midi, "%s queue req: %d\n",
>>   midi->out_ep->name, err);
>> + free_ep_req(midi->out_ep, req);
>>   }
>>   }
>>
>>
>
> Don't we have similar problem in f_midi_transmit() and f_midi_complete()
> functions?

Yes and it is been addressed on Patch 7.

Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 5/7] usb: gadget: f_midi: set altsettings only for MIDIStreaming interface

[Felipe Tonello]

Hi Robert,

On Mon, Oct 26, 2015 at 10:30 PM, Robert Baldyga
 wrote:
> On 10/26/2015 05:55 PM, Felipe F. Tonello wrote:
>> This avoids duplication of USB requests for OUT endpoint and
>> re-enabling endpoints.
>>
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>  drivers/usb/gadget/function/f_midi.c | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c 
>> b/drivers/usb/gadget/function/f_midi.c
>> index 0e9cdeb..a617df3 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -323,6 +323,10 @@ static int f_midi_set_alt(struct usb_function *f, 
>> unsigned intf, unsigned alt)
>>   unsigned i;
>>   int err;
>>
>> + /* We don't care if it is not MIDIStreaming interface */
>> + if (intf != ms_interface_desc.bInterfaceNumber)
>> + return 0;
>> +
>
> These global descriptors are overwritten in bind() of each instance of
> f_midi, so you have no guarantee that your bInterfaceNumber is correct
> for your current instance. Instead you should store value obtained from
> usb_interface_id() during bind().

Ok.

But then this interface descriptors shouldn't be global static,
because they will always reflect the latest bind() only. Right?

>
>>   err = f_midi_start_ep(midi, f, midi->in_ep);
>>   if (err)
>>   return err;
>>

Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/4] usb: gadget: f_midi: free usb request when done

[Felipe Tonello]

Hi Balbi,

On Sun, Oct 11, 2015 at 8:08 PM, Clemens Ladisch <clem...@ladisch.de> wrote:
> Felipe Balbi wrote:
>> Clemens Ladisch <clem...@ladisch.de> writes:
>>> Felipe Tonello wrote:
>>>> req->actual == req->length means that there is no data left to enqueue,
>>>
>>> This condition is not checked in the patch.
>>>
>>>> so free the request.
>>>>
>>>> Signed-off-by: Felipe F. Tonello <e...@felipetonello.com>
>>>> ---
>>>>  drivers/usb/gadget/function/f_midi.c | 5 ++---
>>>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>>> diff --git a/drivers/usb/gadget/function/f_midi.c 
>>>> b/drivers/usb/gadget/function/f_midi.c
>>>> index edb84ca..93212ca 100644
>>>> --- a/drivers/usb/gadget/function/f_midi.c
>>>> +++ b/drivers/usb/gadget/function/f_midi.c
>>>> @@ -256,9 +256,8 @@ f_midi_complete(struct usb_ep *ep, struct usb_request 
>>>> *req)
>>>> /* We received stuff. req is queued again, below */
>>>> f_midi_handle_out_data(ep, req);
>>>> } else if (ep == midi->in_ep) {
>>>> -   /* Our transmit completed. See if there's more to go.
>>>> -* f_midi_transmit eats req, don't queue it again. */
>>>> -   f_midi_transmit(midi, req);
>>>> +   /* Our transmit completed. Don't queue it again. */
>>>> +   free_ep_req(ep, req);
>>>> return;
>>>> }
>>>> break;
>>>
>>> The ALSA framework will give you a notification _once_.  If the
>>> resulting data is larger than midi->buflen, then you have to continue
>>> sending packets.  This is exactly what the call to f_midi_transmit()
>>> does.
>>
>> so, should I drop this series from my TODO queue ?
>
> Yes, this patch needs to be dropped.

You don't need to drop the whole series. Just this patch needs more
work, the rest are fine (including bug fixes).

Felipe Tonello
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/4] usb: gadget: f_midi: free usb request when done

[Felipe Tonello]

Hi Clemens

On Fri, Oct 9, 2015 at 10:23 AM, Clemens Ladisch <clem...@ladisch.de> wrote:
> Felipe Tonello wrote:
>> req->actual == req->length means that there is no data left to enqueue,
>
> This condition is not checked in the patch.
>
>> so free the request.
>>
>> Signed-off-by: Felipe F. Tonello <e...@felipetonello.com>
>> ---
>>  drivers/usb/gadget/function/f_midi.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>> diff --git a/drivers/usb/gadget/function/f_midi.c 
>> b/drivers/usb/gadget/function/f_midi.c
>> index edb84ca..93212ca 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -256,9 +256,8 @@ f_midi_complete(struct usb_ep *ep, struct usb_request 
>> *req)
>>   /* We received stuff. req is queued again, below */
>>   f_midi_handle_out_data(ep, req);
>>   } else if (ep == midi->in_ep) {
>> - /* Our transmit completed. See if there's more to go.
>> -  * f_midi_transmit eats req, don't queue it again. */
>> - f_midi_transmit(midi, req);
>> + /* Our transmit completed. Don't queue it again. */
>> + free_ep_req(ep, req);
>>   return;
>>   }
>>   break;
>
> The ALSA framework will give you a notification _once_.  If the
> resulting data is larger than midi->buflen, then you have to continue
> sending packets.  This is exactly what the call to f_midi_transmit()
> does.

That's true. But what it will also happen is that f_midi_transmit()
will potentially eat up data from an unrelated ALSA trigger.
In the end it is all fine, because the function will realise the
request->len == 0 so it will free the request. But logically speaking
it is incorrect and error prone.

>
> (To decrease latency, it might be a good idea to queue multiple requests,
> but you wouldn't want to continue piling up requests if the host isn't
> listening.  sound/usb/midi.c just allocates a fixed number of requests,
> and always reuses them.)

I believe that is the best way to implement. Create multiple requests
until the ALSA substreams buffer are empty and free the request on
completion.

The problem of having requests when host isn't listening will happen
anyway because there is no way to know that until completion.

What do you think?

Felipe Tonello
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/4] usb: gadget: f_midi: free usb request when done

[Felipe Tonello]

Hi Clemens

On Mon, Oct 12, 2015 at 11:16 AM, Clemens Ladisch <clem...@ladisch.de> wrote:
> Felipe Tonello wrote:
>> On Fri, Oct 9, 2015 at 10:23 AM, Clemens Ladisch <clem...@ladisch.de> wrote:
>>> Felipe Tonello wrote:
>>>>   } else if (ep == midi->in_ep) {
>>>> - /* Our transmit completed. See if there's more to go.
>>>> -  * f_midi_transmit eats req, don't queue it again. */
>>>> - f_midi_transmit(midi, req);
>>>> + /* Our transmit completed. Don't queue it again. */
>>>> + free_ep_req(ep, req);
>>>>   return;
>>>>   }
>>>>   break;
>>>
>>> The ALSA framework will give you a notification _once_.  If the
>>> resulting data is larger than midi->buflen, then you have to continue
>>> sending packets.  This is exactly what the call to f_midi_transmit()
>>> does.
>>
>> That's true. But what it will also happen is that f_midi_transmit()
>> will potentially eat up data from an unrelated ALSA trigger.
>
> The triggers are for some MIDI port of the _same_ USB endpoint, so
> they're not unrelated.  f_midi_transmit() collects data from all ports
> anyway; separating the data from different ports into different USB
> packets would just needlessly introduce additional latency.

Ok.

>
>> In the end it is all fine, because the function will realise the
>> request->len == 0 so it will free the request. But logically speaking
>> it is incorrect and error prone.
>
> It is _not_ incorrect if you realize that f_midi_transmit() applies to
> the endpoint, not to any particular port.
>
>>> (To decrease latency, it might be a good idea to queue multiple requests,
>>> but you wouldn't want to continue piling up requests if the host isn't
>>> listening.  sound/usb/midi.c just allocates a fixed number of requests,
>>> and always reuses them.)
>>
>> I believe that is the best way to implement. Create multiple requests
>> until the ALSA substreams buffer are empty and free the request on
>> completion.
>
> I believe a better way to implement this is to allocate a fixed number
> of requests, and to always reuse them.

How many?

>
>> The problem of having requests when host isn't listening will happen
>> anyway because there is no way to know that until completion.
>
> But if you have no upper limit on the number of queues requests, you
> will eventually run out of (DMA) memory.

And that's what happening at the moment. One of my patches are to fix
a memory leak when that happens.

But it would be ideal to have a FIFO of requests and perhaps ignore
new requests if the FIFO is full.

So, allocate (pre-allocate?) new requests until the FIFO is full. Upon
completion, remove the request from FIFO, but still reuse it on
f_midi_transmit() and queue it on the FIFO again if there is still
data from ALSA, otherwise just free the request.

What do you think?

Felipe Tonello
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/4] USB MIDI Gadget bug fixes and improvements

[Felipe Tonello]

On Tue, Sep 29, 2015 at 1:01 PM, Felipe F. Tonello  
wrote:
> Here is the third version of this patch set. It includes memory leakage bug
> fix, improvements and code cleanups.
>
> Felipe F. Tonello (4):
>   usb: gadget: f_midi: free usb request when done
>   usb: gadget: f_midi: free request when usb_ep_queue fails
>   usb: gadget: f_midi: Transmit data only when IN ep is enabled
>   usb: gadget: f_midi: remove duplicated code
>
>  drivers/usb/gadget/function/f_midi.c | 36 
> +++-
>  1 file changed, 11 insertions(+), 25 deletions(-)
>
> --
> 2.1.4
>

ping?
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] usb: gadget: f_midi: free request when usb_ep_queue fails

[Felipe Tonello]

On Thu, Sep 24, 2015 at 2:20 AM, Peter Chen <peter.c...@freescale.com> wrote:
> On Wed, Sep 23, 2015 at 12:40:46PM +0100, Felipe Tonello wrote:
>> Hi Peter,
>>
>> On Wed, Sep 23, 2015 at 8:09 AM, Peter Chen <peter.c...@freescale.com> wrote:
>> > On Tue, Sep 22, 2015 at 07:59:10PM +0100, Felipe F. Tonello wrote:
>> >> This fix a memory leak that will occur in this case.
>> >>
>> >> Signed-off-by: Felipe F. Tonello <e...@felipetonello.com>
>> >> ---
>> >>  drivers/usb/gadget/function/f_midi.c | 4 +++-
>> >>  1 file changed, 3 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/usb/gadget/function/f_midi.c 
>> >> b/drivers/usb/gadget/function/f_midi.c
>> >> index e92aff5..e6a114b 100644
>> >> --- a/drivers/usb/gadget/function/f_midi.c
>> >> +++ b/drivers/usb/gadget/function/f_midi.c
>> >> @@ -550,9 +550,11 @@ static void f_midi_transmit(struct f_midi *midi, 
>> >> struct usb_request *req)
>> >>   int err;
>> >>
>> >>   err = usb_ep_queue(ep, req, GFP_ATOMIC);
>> >> - if (err < 0)
>> >> + if (err < 0) {
>> >>   ERROR(midi, "%s queue req: %d\n",
>> >> midi->in_ep->name, err);
>> >> + free_ep_req(ep, req);
>> >> + }
>> >>   } else {
>> >>   free_ep_req(ep, req);
>> >>   }
>> >> --
>> >> 2.1.4
>> >>
>> >
>> > I may know your problem, current midi library, alsa and this driver
>> > allow device sends as much data as possible, but without block the
>> > sending until host reads data, it only allocates the request buffer
>> > (using midi_alloc_ep_req), but without free, so after you send
>> > enough data, it is out of memory.
>>
>> Yes. Also there is the case where the usb cable is not conected, thus
>> failing to hardware enqueue the request, causing a memory leak on this
>> request.
>>
>
> If the usb cable is not connected, the related endpoints should be
> not enabled. Would you really observe enqueue the request without
> cable connected?

The usb_ep_queue() returns an error if it is not connected, causing
the request never to be freed and never to be queued. Thus a memory
leak happens.

Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] usb: chipidea: core: fix when building without CONFIG_PM support

[Felipe Tonello]

Hi Peter,

On Thu, Sep 24, 2015 at 2:17 AM, Peter Chen  wrote:
> On Wed, Sep 23, 2015 at 12:56:58PM +0100, Felipe F. Tonello wrote:
>> If CONFIG_PM or CONFIG_PM_SLEEP is not set, driver will not compile
>> properly.
>>
>
> Would you post the warning or error messages?
>
> I just tried at v4.3-rc1 (v4.2 should be same), without any problems.

Actually I tested again with the latest and it doesn't break. But
still I believe it is the right thing to do, even though it builds.
Just good practice to make sure the ifdefs are correct on driver code.

Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] usb: gadget: f_midi: free usb request when done

[Felipe Tonello]

Hi Peter,

On Thu, Sep 24, 2015 at 2:38 AM, Peter Chen  wrote:
> On Wed, Sep 23, 2015 at 01:01:44PM +0100, Felipe F. Tonello wrote:
>> req->actual == req->length means that there is no data left to enqueue,
>> so free the request.
>>
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>
>> Changes in v2:
>>  * Re enqueue not fully completed requests, instead of read ALSA buffers.
>>
>>  drivers/usb/gadget/function/f_midi.c | 10 ++
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c 
>> b/drivers/usb/gadget/function/f_midi.c
>> index edb84ca..62356cf 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -256,10 +256,12 @@ f_midi_complete(struct usb_ep *ep, struct usb_request 
>> *req)
>>   /* We received stuff. req is queued again, below */
>>   f_midi_handle_out_data(ep, req);
>>   } else if (ep == midi->in_ep) {
>> - /* Our transmit completed. See if there's more to go.
>> -  * f_midi_transmit eats req, don't queue it again. */
>> - f_midi_transmit(midi, req);
>> - return;
>> + /* Our transmit completed. If there is no more to go,
>> +don't queue it again. */
>> + if (req->actual == req->length) {
>> + free_ep_req(ep, req);
>> + return;
>> + }
>
> I find f_midi_transmit will judge if there are more data, if without
> data, it will free the request.

No. Transmit is called when ALSA trigger is called (whenever
snd_*_read() snd_*_write() is called).

The current code is not crashing anything, but I believe it is just
wrong. It can cause potential problems in future if one is not careful
when doing changes to it.

I would like to send a v3 with free_ep_req().

Felipe

>
> Besides, does one trigger only do one transfer? Sorry, I can't make my
> aplaymidi to receive data, probably due to hardware without midi
> support?
>
> root@imx6qpsabreauto:~# aplaymidi
> ALSA lib
> /var/lib/jenkins/workspace/fido-3.14.28-X11-consolidated_new/temp_build_dir/build_all/tmp/work/cortexa9hf-vfp-neon-mx6qdl-poky-linux-gnueabi/alsa-lib/1.0.28-r0/alsa-lib-1.0.28/src/seq/seq_hw.c:457:(snd_seq_hw_open)
> open /dev/snd/seq failed: No such file or directory
> Cannot open sequencer - No such file or directory
>
> --
>
> Best Regards,
> Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Problems with printk logs and my driver

[Felipe Tonello]

On Thu, Sep 24, 2015 at 9:51 PM, Alan Stern  wrote:
> On Thu, 24 Sep 2015, Jiri Kosina wrote:
>
>> On Wed, 23 Sep 2015, Alan Stern wrote:
>>
>> > Your mistake was thinking that the driver for your keyboard is usbkbd.
>> > It isn't.  It's usbhid, as you can see in the "lsusb -t" output above.
>>
>> As Eric is absolutely not the first person ever who got confused by this
>> (and I can certainly understand the reasons for this confusion), I've been
>> thinking for quite some time already about renaming this driver (and
>> usbmouse as well). We'd probably want to make obvious from the name that
>> this isn't regular usb keyboard driver.
>>
>> Any opinions on usbkbd-simple or usbkbd-dummy? The most accurate would of
>> course be usbkbd-boot, but that might be equally confusing.
>
> I prefer -simple over -dummy.  Even better would be usbkbd-alt.
>
>> The drawback I can see in renaming the driver is various embedded folks
>> having he name hardcoded in their scripts.
>
> Yes, that's a real problem.  Similar considerations apply to .config
> files, if you also change the Kconfig symbol name.

Maybe a better description on Kconfig and/or comments on source code
it's enough.

Because breaking that compatibility just because of a name doesn't
sound really appealing. I had previously denied patches for breaking
compatibility by making things correct.

Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] usb: gadget: f_midi: free request when usb_ep_queue fails

[Felipe Tonello]

On Fri, Sep 25, 2015 at 10:02 AM, Peter Chen <peter.c...@freescale.com> wrote:
> On Fri, Sep 25, 2015 at 09:27:49AM +0100, Felipe Tonello wrote:
>> On Thu, Sep 24, 2015 at 2:20 AM, Peter Chen <peter.c...@freescale.com> wrote:
>> > On Wed, Sep 23, 2015 at 12:40:46PM +0100, Felipe Tonello wrote:
>> >> Hi Peter,
>> >>
>> >> On Wed, Sep 23, 2015 at 8:09 AM, Peter Chen <peter.c...@freescale.com> 
>> >> wrote:
>> >> > On Tue, Sep 22, 2015 at 07:59:10PM +0100, Felipe F. Tonello wrote:
>> >> >> This fix a memory leak that will occur in this case.
>> >> >>
>> >> >> Signed-off-by: Felipe F. Tonello <e...@felipetonello.com>
>> >> >> ---
>> >> >>  drivers/usb/gadget/function/f_midi.c | 4 +++-
>> >> >>  1 file changed, 3 insertions(+), 1 deletion(-)
>> >> >>
>> >> >> diff --git a/drivers/usb/gadget/function/f_midi.c 
>> >> >> b/drivers/usb/gadget/function/f_midi.c
>> >> >> index e92aff5..e6a114b 100644
>> >> >> --- a/drivers/usb/gadget/function/f_midi.c
>> >> >> +++ b/drivers/usb/gadget/function/f_midi.c
>> >> >> @@ -550,9 +550,11 @@ static void f_midi_transmit(struct f_midi *midi, 
>> >> >> struct usb_request *req)
>> >> >>   int err;
>> >> >>
>> >> >>   err = usb_ep_queue(ep, req, GFP_ATOMIC);
>> >> >> - if (err < 0)
>> >> >> + if (err < 0) {
>> >> >>   ERROR(midi, "%s queue req: %d\n",
>> >> >> midi->in_ep->name, err);
>> >> >> + free_ep_req(ep, req);
>> >> >> + }
>> >> >>   } else {
>> >> >>   free_ep_req(ep, req);
>> >> >>   }
>> >> >> --
>> >> >> 2.1.4
>> >> >>
>> >> >
>> >> > I may know your problem, current midi library, alsa and this driver
>> >> > allow device sends as much data as possible, but without block the
>> >> > sending until host reads data, it only allocates the request buffer
>> >> > (using midi_alloc_ep_req), but without free, so after you send
>> >> > enough data, it is out of memory.
>> >>
>> >> Yes. Also there is the case where the usb cable is not conected, thus
>> >> failing to hardware enqueue the request, causing a memory leak on this
>> >> request.
>> >>
>> >
>> > If the usb cable is not connected, the related endpoints should be
>> > not enabled. Would you really observe enqueue the request without
>> > cable connected?
>>
>> The usb_ep_queue() returns an error if it is not connected, causing
>> the request never to be freed and never to be queued. Thus a memory
>> leak happens.
>>
>
> If it is not connected, the ep is not enabled, why we will call
> usb_ep_queue? If it really does, there must be something wrong.

Ok. I can do another patch to check for that. But this patch still
applies, because if for some reason the usb_ep_queue() returns an
error, it will not leak memory.

Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: MIDI gadget allocating too much from coherent pool

[Felipe Tonello]

On Wed, Sep 23, 2015 at 12:39 PM, Felipe Tonello <e...@felipetonello.com> wrote:
> Hi Peter,
>
> On Wed, Sep 23, 2015 at 4:09 AM, Peter Chen <peter.c...@freescale.com> wrote:
>> On Tue, Sep 22, 2015 at 07:51:34PM +0100, Felipe Tonello wrote:
>>> Hi all,
>>>
>>> On Tue, Sep 22, 2015 at 10:13 AM, Felipe Tonello <e...@felipetonello.com> 
>>> wrote:
>>> > Hi Peter,
>>> >
>>> > On Tue, Sep 22, 2015 at 8:03 AM, Peter Chen <peter.c...@freescale.com> 
>>> > wrote:
>>> >> On Tue, Sep 22, 2015 at 09:07:23AM +0100, Felipe Tonello wrote:
>>> >>> On Tue, Sep 22, 2015 at 12:41 AM, Peter Chen <peter.c...@freescale.com> 
>>> >>> wrote:
>>> >>> > On Mon, Sep 21, 2015 at 03:25:28PM +0100, Felipe Tonello wrote:
>>> >>> >> Hi all,
>>> >>> >>
>>> >>> >> I actually found the problem but can't really understand. The 
>>> >>> >> ci_irq()
>>> >>> >> handler (from core.c) is not been called after a ep_queue() from
>>> >>> >> f_midi_transmit().
>>> >>> >>
>>> >>> >> Is there any reason for that?
>>> >>> >>
>>> >>> >> I used mass_storage gadget, made file transfers and others, and the
>>> >>> >> interrupt handler was been called as expected.
>>> >>> >>
>>> >>> >
>>> >>> > Which Soc are you using? And which kernel version are you using?
>>> >>>
>>> >>> i.MX6Q (industrial temp) and v4.2. We are using the imx6 REX module[1].
>>> >>>
>>> >>> We checked the errata and didn't seem to have anything relevant.
>>> >>>
>>> >>> I wonder: was f_midi ever working properly, ie, complete callback ever 
>>> >>> called?
>>> >>>
>>> >>
>>> >> Would you give your cpu revision number, and show me
>>> >> how to reproduce it? I can test at my board.
>>> >
>>> > MCIMX6QAVT10AC
>>> >
>>> > To reproduce:
>>> >  * add this line to the f_midi_complete() function under the "case 0":
>>> >
>>> > VDBG(cdev, "%s normal completion (%d), %d/%d\n", ep->name, status,
>>> > req->actual, req->length);
>>> >
>>> >  * build a kernel with verbose debug enabled on USB gadget subsystem
>>> >  * load g_ether module (this will create an ALSA card and device)
>>> >  * connect device to host via usb otg cable.
>>> >  * to list the ALSA device, run `amidi -l', use the device listed as 
>>> > "f_midi"
>>> >  * send midi message using `amidi -p hw:1,0 -S 901010', my device is
>>> > hw:1,0, check the output of amidi -l.
>>> >  * run `dmesg'  you should see the message above, but if doesn't then
>>> > probably the complete callback wasn't called as well.
>>> >
>>> > OBS: We have set the OTG_ID pin to type B (device), so no need to OTG
>>> > cable on our side.
>>>
>>> I realized that when the device is connected to the host but the host
>>> is not reading data, the device's interrupt will never be triggered.
>>> Is that what is supposed to happen?
>>>
>>> For example: if I send lots of data via `amidi -s' from the device to
>>> the host, but until I run `amidi -d' (which dumps data from buffer) on
>>> the host the interrupt on the device is never triggered.
>>>
>>> I will send two or three small patches that improve the situation.
>>> Freeing the request when not needed any more.
>>>
>>
>> No, it is supposed. If the device does not queue request before host
>> sends data, the device can't know when the host sends data.
>
> That's why my patch 2 is necessary.

Sorry, patch number 3. Although I feel that number 2 is also an
improvement, but not bug fix.

Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] usb: gadget: f_midi: free usb request when done

[Felipe Tonello]

Hi Peter,

On Wed, Sep 23, 2015 at 4:10 AM, Peter Chen  wrote:
> On Tue, Sep 22, 2015 at 07:59:09PM +0100, Felipe F. Tonello wrote:
>> req->actual == req->length means that there is no data left to enqueue,
>> so free the request.
>>
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>  drivers/usb/gadget/function/f_midi.c | 5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c 
>> b/drivers/usb/gadget/function/f_midi.c
>> index edb84ca..e92aff5 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -258,7 +258,10 @@ f_midi_complete(struct usb_ep *ep, struct usb_request 
>> *req)
>>   } else if (ep == midi->in_ep) {
>>   /* Our transmit completed. See if there's more to go.
>>* f_midi_transmit eats req, don't queue it again. */
>> - f_midi_transmit(midi, req);
>> + if (req->actual < req->length)
>> + f_midi_transmit(midi, req);
>> + else
>> + free_ep_req(ep, req);
>>   return;
>>   }
>
> It is incorrect, if no reqeust in queue, how device knows when
> the host sends data?

This is the complete function of the IN endpoint.

Actually I believe the proper patch is to enqueue this request again
if req->actual < req->length is true. Because the data is still there,
just not fully completed. Asking to transmit the request again will
cause to read new data from ALSA MIDI module, which it can possibly
steal data from a real ALSA request from f_midi_in_trigger. If that
doesn't happen (req->length == 0), the request will be freed anyway.

Any thoughts?

Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] usb: gadget: f_midi: free request when usb_ep_queue fails

[Felipe Tonello]

Hi Peter,

On Wed, Sep 23, 2015 at 8:09 AM, Peter Chen  wrote:
> On Tue, Sep 22, 2015 at 07:59:10PM +0100, Felipe F. Tonello wrote:
>> This fix a memory leak that will occur in this case.
>>
>> Signed-off-by: Felipe F. Tonello 
>> ---
>>  drivers/usb/gadget/function/f_midi.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_midi.c 
>> b/drivers/usb/gadget/function/f_midi.c
>> index e92aff5..e6a114b 100644
>> --- a/drivers/usb/gadget/function/f_midi.c
>> +++ b/drivers/usb/gadget/function/f_midi.c
>> @@ -550,9 +550,11 @@ static void f_midi_transmit(struct f_midi *midi, struct 
>> usb_request *req)
>>   int err;
>>
>>   err = usb_ep_queue(ep, req, GFP_ATOMIC);
>> - if (err < 0)
>> + if (err < 0) {
>>   ERROR(midi, "%s queue req: %d\n",
>> midi->in_ep->name, err);
>> + free_ep_req(ep, req);
>> + }
>>   } else {
>>   free_ep_req(ep, req);
>>   }
>> --
>> 2.1.4
>>
>
> I may know your problem, current midi library, alsa and this driver
> allow device sends as much data as possible, but without block the
> sending until host reads data, it only allocates the request buffer
> (using midi_alloc_ep_req), but without free, so after you send
> enough data, it is out of memory.

Yes. Also there is the case where the usb cable is not conected, thus
failing to hardware enqueue the request, causing a memory leak on this
request.

Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: MIDI gadget allocating too much from coherent pool

[Felipe Tonello]

Hi Peter,

On Wed, Sep 23, 2015 at 4:09 AM, Peter Chen <peter.c...@freescale.com> wrote:
> On Tue, Sep 22, 2015 at 07:51:34PM +0100, Felipe Tonello wrote:
>> Hi all,
>>
>> On Tue, Sep 22, 2015 at 10:13 AM, Felipe Tonello <e...@felipetonello.com> 
>> wrote:
>> > Hi Peter,
>> >
>> > On Tue, Sep 22, 2015 at 8:03 AM, Peter Chen <peter.c...@freescale.com> 
>> > wrote:
>> >> On Tue, Sep 22, 2015 at 09:07:23AM +0100, Felipe Tonello wrote:
>> >>> On Tue, Sep 22, 2015 at 12:41 AM, Peter Chen <peter.c...@freescale.com> 
>> >>> wrote:
>> >>> > On Mon, Sep 21, 2015 at 03:25:28PM +0100, Felipe Tonello wrote:
>> >>> >> Hi all,
>> >>> >>
>> >>> >> I actually found the problem but can't really understand. The ci_irq()
>> >>> >> handler (from core.c) is not been called after a ep_queue() from
>> >>> >> f_midi_transmit().
>> >>> >>
>> >>> >> Is there any reason for that?
>> >>> >>
>> >>> >> I used mass_storage gadget, made file transfers and others, and the
>> >>> >> interrupt handler was been called as expected.
>> >>> >>
>> >>> >
>> >>> > Which Soc are you using? And which kernel version are you using?
>> >>>
>> >>> i.MX6Q (industrial temp) and v4.2. We are using the imx6 REX module[1].
>> >>>
>> >>> We checked the errata and didn't seem to have anything relevant.
>> >>>
>> >>> I wonder: was f_midi ever working properly, ie, complete callback ever 
>> >>> called?
>> >>>
>> >>
>> >> Would you give your cpu revision number, and show me
>> >> how to reproduce it? I can test at my board.
>> >
>> > MCIMX6QAVT10AC
>> >
>> > To reproduce:
>> >  * add this line to the f_midi_complete() function under the "case 0":
>> >
>> > VDBG(cdev, "%s normal completion (%d), %d/%d\n", ep->name, status,
>> > req->actual, req->length);
>> >
>> >  * build a kernel with verbose debug enabled on USB gadget subsystem
>> >  * load g_ether module (this will create an ALSA card and device)
>> >  * connect device to host via usb otg cable.
>> >  * to list the ALSA device, run `amidi -l', use the device listed as 
>> > "f_midi"
>> >  * send midi message using `amidi -p hw:1,0 -S 901010', my device is
>> > hw:1,0, check the output of amidi -l.
>> >  * run `dmesg'  you should see the message above, but if doesn't then
>> > probably the complete callback wasn't called as well.
>> >
>> > OBS: We have set the OTG_ID pin to type B (device), so no need to OTG
>> > cable on our side.
>>
>> I realized that when the device is connected to the host but the host
>> is not reading data, the device's interrupt will never be triggered.
>> Is that what is supposed to happen?
>>
>> For example: if I send lots of data via `amidi -s' from the device to
>> the host, but until I run `amidi -d' (which dumps data from buffer) on
>> the host the interrupt on the device is never triggered.
>>
>> I will send two or three small patches that improve the situation.
>> Freeing the request when not needed any more.
>>
>
> No, it is supposed. If the device does not queue request before host
> sends data, the device can't know when the host sends data.

That's why my patch 2 is necessary.

Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] usb: gadget: f_midi: free usb request when done

[Felipe Tonello]

Hi Alan,

On Wed, Sep 23, 2015 at 3:30 PM, Alan Stern <st...@rowland.harvard.edu> wrote:
> On Wed, 23 Sep 2015, Felipe Tonello wrote:
>
>> Hi Peter,
>>
>> On Wed, Sep 23, 2015 at 4:10 AM, Peter Chen <peter.c...@freescale.com> wrote:
>> > On Tue, Sep 22, 2015 at 07:59:09PM +0100, Felipe F. Tonello wrote:
>> >> req->actual == req->length means that there is no data left to enqueue,
>> >> so free the request.
>> >>
>> >> Signed-off-by: Felipe F. Tonello <e...@felipetonello.com>
>> >> ---
>> >>  drivers/usb/gadget/function/f_midi.c | 5 -
>> >>  1 file changed, 4 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/usb/gadget/function/f_midi.c 
>> >> b/drivers/usb/gadget/function/f_midi.c
>> >> index edb84ca..e92aff5 100644
>> >> --- a/drivers/usb/gadget/function/f_midi.c
>> >> +++ b/drivers/usb/gadget/function/f_midi.c
>> >> @@ -258,7 +258,10 @@ f_midi_complete(struct usb_ep *ep, struct 
>> >> usb_request *req)
>> >>   } else if (ep == midi->in_ep) {
>> >>   /* Our transmit completed. See if there's more to 
>> >> go.
>> >>* f_midi_transmit eats req, don't queue it again. 
>> >> */
>> >> - f_midi_transmit(midi, req);
>> >> + if (req->actual < req->length)
>> >> + f_midi_transmit(midi, req);
>> >> + else
>> >> + free_ep_req(ep, req);
>> >>   return;
>> >>   }
>> >
>> > It is incorrect, if no reqeust in queue, how device knows when
>> > the host sends data?
>>
>> This is the complete function of the IN endpoint.
>>
>> Actually I believe the proper patch is to enqueue this request again
>> if req->actual < req->length is true. Because the data is still there,
>> just not fully completed. Asking to transmit the request again will
>> cause to read new data from ALSA MIDI module, which it can possibly
>> steal data from a real ALSA request from f_midi_in_trigger. If that
>> doesn't happen (req->length == 0), the request will be freed anyway.
>>
>> Any thoughts?
>
> Please pardon me for jumping in in the middle of a conversation.  I
> know practically zero about f_midi.  But nevertheless...
>
> How can you ever have req->actual < req->length for a usb_request on an
> IN endpoint?  The only way that can happen is if some sort of error or
> exceptional event occurred, for example, if the transfer was cancelled
> before it could run to completion.  In such cases I doubt that you
> really want to retransmit the data.  Particularly since part of it
> probably was received by the host -- do you really want to send that
> part of the data a second time?

That is a fair point.

IMO we should always free the request upon a completion. Never
retransmit, since ALSA trigger will do that anyway.

Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: MIDI gadget allocating too much from coherent pool

[Felipe Tonello]

Hi all,

On Tue, Sep 22, 2015 at 10:13 AM, Felipe Tonello <e...@felipetonello.com> wrote:
> Hi Peter,
>
> On Tue, Sep 22, 2015 at 8:03 AM, Peter Chen <peter.c...@freescale.com> wrote:
>> On Tue, Sep 22, 2015 at 09:07:23AM +0100, Felipe Tonello wrote:
>>> On Tue, Sep 22, 2015 at 12:41 AM, Peter Chen <peter.c...@freescale.com> 
>>> wrote:
>>> > On Mon, Sep 21, 2015 at 03:25:28PM +0100, Felipe Tonello wrote:
>>> >> Hi all,
>>> >>
>>> >> I actually found the problem but can't really understand. The ci_irq()
>>> >> handler (from core.c) is not been called after a ep_queue() from
>>> >> f_midi_transmit().
>>> >>
>>> >> Is there any reason for that?
>>> >>
>>> >> I used mass_storage gadget, made file transfers and others, and the
>>> >> interrupt handler was been called as expected.
>>> >>
>>> >
>>> > Which Soc are you using? And which kernel version are you using?
>>>
>>> i.MX6Q (industrial temp) and v4.2. We are using the imx6 REX module[1].
>>>
>>> We checked the errata and didn't seem to have anything relevant.
>>>
>>> I wonder: was f_midi ever working properly, ie, complete callback ever 
>>> called?
>>>
>>
>> Would you give your cpu revision number, and show me
>> how to reproduce it? I can test at my board.
>
> MCIMX6QAVT10AC
>
> To reproduce:
>  * add this line to the f_midi_complete() function under the "case 0":
>
> VDBG(cdev, "%s normal completion (%d), %d/%d\n", ep->name, status,
> req->actual, req->length);
>
>  * build a kernel with verbose debug enabled on USB gadget subsystem
>  * load g_ether module (this will create an ALSA card and device)
>  * connect device to host via usb otg cable.
>  * to list the ALSA device, run `amidi -l', use the device listed as "f_midi"
>  * send midi message using `amidi -p hw:1,0 -S 901010', my device is
> hw:1,0, check the output of amidi -l.
>  * run `dmesg'  you should see the message above, but if doesn't then
> probably the complete callback wasn't called as well.
>
> OBS: We have set the OTG_ID pin to type B (device), so no need to OTG
> cable on our side.

I realized that when the device is connected to the host but the host
is not reading data, the device's interrupt will never be triggered.
Is that what is supposed to happen?

For example: if I send lots of data via `amidi -s' from the device to
the host, but until I run `amidi -d' (which dumps data from buffer) on
the host the interrupt on the device is never triggered.

I will send two or three small patches that improve the situation.
Freeing the request when not needed any more.

Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: MIDI gadget allocating too much from coherent pool

[Felipe Tonello]

Hi Peter,

On Tue, Sep 22, 2015 at 8:03 AM, Peter Chen <peter.c...@freescale.com> wrote:
> On Tue, Sep 22, 2015 at 09:07:23AM +0100, Felipe Tonello wrote:
>> On Tue, Sep 22, 2015 at 12:41 AM, Peter Chen <peter.c...@freescale.com> 
>> wrote:
>> > On Mon, Sep 21, 2015 at 03:25:28PM +0100, Felipe Tonello wrote:
>> >> Hi all,
>> >>
>> >> I actually found the problem but can't really understand. The ci_irq()
>> >> handler (from core.c) is not been called after a ep_queue() from
>> >> f_midi_transmit().
>> >>
>> >> Is there any reason for that?
>> >>
>> >> I used mass_storage gadget, made file transfers and others, and the
>> >> interrupt handler was been called as expected.
>> >>
>> >
>> > Which Soc are you using? And which kernel version are you using?
>>
>> i.MX6Q (industrial temp) and v4.2. We are using the imx6 REX module[1].
>>
>> We checked the errata and didn't seem to have anything relevant.
>>
>> I wonder: was f_midi ever working properly, ie, complete callback ever 
>> called?
>>
>
> Would you give your cpu revision number, and show me
> how to reproduce it? I can test at my board.

MCIMX6QAVT10AC

To reproduce:
 * add this line to the f_midi_complete() function under the "case 0":

VDBG(cdev, "%s normal completion (%d), %d/%d\n", ep->name, status,
req->actual, req->length);

 * build a kernel with verbose debug enabled on USB gadget subsystem
 * load g_ether module (this will create an ALSA card and device)
 * connect device to host via usb otg cable.
 * to list the ALSA device, run `amidi -l', use the device listed as "f_midi"
 * send midi message using `amidi -p hw:1,0 -S 901010', my device is
hw:1,0, check the output of amidi -l.
 * run `dmesg'  you should see the message above, but if doesn't then
probably the complete callback wasn't called as well.

OBS: We have set the OTG_ID pin to type B (device), so no need to OTG
cable on our side.

Thanks,

Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: MIDI gadget allocating too much from coherent pool

[Felipe Tonello]

On Tue, Sep 22, 2015 at 12:41 AM, Peter Chen <peter.c...@freescale.com> wrote:
> On Mon, Sep 21, 2015 at 03:25:28PM +0100, Felipe Tonello wrote:
>> Hi all,
>>
>> I actually found the problem but can't really understand. The ci_irq()
>> handler (from core.c) is not been called after a ep_queue() from
>> f_midi_transmit().
>>
>> Is there any reason for that?
>>
>> I used mass_storage gadget, made file transfers and others, and the
>> interrupt handler was been called as expected.
>>
>
> Which Soc are you using? And which kernel version are you using?

i.MX6Q (industrial temp) and v4.2. We are using the imx6 REX module[1].

We checked the errata and didn't seem to have anything relevant.

I wonder: was f_midi ever working properly, ie, complete callback ever called?

[1] http://www.imx6rex.com/

Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] usb: gadget: f_midi: check for error on usb_ep_queue

[Felipe Tonello]

Hi Chen,

On Mon, Sep 21, 2015 at 7:30 AM, Peter Chen  wrote:
> On Fri, Sep 18, 2015 at 06:12:41PM +0100, e...@felipetonello.com wrote:
>> From: "Felipe F. Tonello" 
>>
>> f_midi is not checking weather the is an error on usb_ep_queue
>
> %s/weather/whether
> %s/the/there

I fixed it on v3. Did you receive it?

Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: MIDI gadget allocating too much from coherent pool

[Felipe Tonello]

Hi all,

I actually found the problem but can't really understand. The ci_irq()
handler (from core.c) is not been called after a ep_queue() from
f_midi_transmit().

Is there any reason for that?

I used mass_storage gadget, made file transfers and others, and the
interrupt handler was been called as expected.

Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: chipidea: udc: improve error handling on ep_queue

[Felipe Tonello]

Hi Peter,

On Mon, Sep 21, 2015 at 7:29 AM, Peter Chen  wrote:
> On Fri, Sep 18, 2015 at 06:12:40PM +0100, e...@felipetonello.com wrote:
>> From: "Felipe F. Tonello" 
>>
>> _ep_queue() didn't check for errors when using add_td_to_list()
>> which can fail if dma_pool_alloc fails, thus causing a kernel
>
> Would you find the root cause why dma_pool_alloc fails?

Partially. For some reason the udc_irq() is not been called after a
f_midi_transmit(). I am looking into that.

Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] usb: gadget: f_midi: check for error on usb_ep_queue

[Felipe Tonello]

Hi Peter,

On Mon, Sep 21, 2015 at 8:49 AM, Peter Chen <peter.c...@freescale.com> wrote:
> On Mon, Sep 21, 2015 at 09:16:05AM +0100, Felipe Tonello wrote:
>> Hi Chen,
>>
>> On Mon, Sep 21, 2015 at 7:30 AM, Peter Chen <peter.c...@freescale.com> wrote:
>> > On Fri, Sep 18, 2015 at 06:12:41PM +0100, e...@felipetonello.com wrote:
>> >> From: "Felipe F. Tonello" <e...@felipetonello.com>
>> >>
>> >> f_midi is not checking weather the is an error on usb_ep_queue
>> >
>> > %s/weather/whether
>> > %s/the/there
>>
>> I fixed it on v3. Did you receive it?
>>
>
> oh, get it, You may add "--subject-prefix="PATCH v3" when you format
> patch, in that case, the reader can see "PATCH v3 2/2" at the subject,
> and know it is the third version.
>
> Your two changes for chipidea are ok, I will queue them.

Thanks. Sorry about that, I thought I did it.

Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] usb: gadget: f_midi: check for error on usb_ep_queue

[Felipe Tonello]

Hi Balbi,

On Fri, Sep 18, 2015 at 6:36 PM,  <e...@felipetonello.com> wrote:
> From: "Felipe F. Tonello" <e...@felipetonello.com>
>
> f_midi is not checking whether there is an error on usb_ep_queue
> request, ignoring potential problems, such as memory leaks.
>
> Signed-off-by: Felipe F. Tonello <e...@felipetonello.com>
> ---
>
> Changes for v2:
>   - Update code style.
>
> Changes for v3:
>   - Use ip_ep instead of out_ep. Fixed typo in commit message.

I forgot to add v3 to the patch subject, so it queued here instead. Do
you want me to re-send as v3?

Felipe Tonello
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: USB client crash on Vybrid with USB gadget RNDIS connection

[Felipe Tonello]

Hi Peter,

On Fri, Sep 18, 2015 at 6:39 AM, Peter Chen  wrote:
> On Wed, Sep 16, 2015 at 02:48:50PM +0530, maitysancha...@gmail.com wrote:
>> On 15-09-16 15:54:21, Peter Chen wrote:
>> > On Wed, Sep 16, 2015 at 02:18:49PM +0530, maitysancha...@gmail.com wrote:
>> > > Hello Peter,
>> > >
>> > > >
>> > > > Enable CONFIG_DEBUG_LIST, it has below position if you
>> > > > run make menuconfig
>> > > > Kernel hacking  --->
>> > > > [*] Debug linked list manipulation
>> > > >
>> > >
>> > > Sorry for the delay. When I enabled this config the first time my test
>> > > application ran for 24 hours or so and I did not get any stack traces.
>> > >
>> > > I restarted the test again and finally got the trace below. You were
>> > > spot on, its a list corruption issue. I modified the trace a bit after
>> > > copying to remove the sprinkled debug messages throughout the trace
>> > > from my test application.
>> > >
>> > > [  622.204134] WARNING: CPU: 0 PID: 0 at lib/list_debug.c:59 
>> > > __list_del_entry+0xc4/0xe8()
>> > > [  622.212870] list_del corruption. prev->next should be 8db63600, but 
>> > > was 36008db6
>> >
>> > You see the higher 16 bits were swapped with lower 16 bits, and the
>> > virtual memory address should begin from 0x8, right?
>>
>> Yes, I saw that but beats me how this happens.
>>
>> >
>> > Check with Vybrid errata to see if all ARM/memory system have applied.
>>
>> What do you mean by "all ARM/memory system have applied" ? I checked with 
>> the Vybrid errata
>> and I do not see anything related.
>>
>
> Just system level errata, like ARM Cortex A5, memory (L1/L2 Cache), etc.
>
> Would you please do more tests to see if the error pattern is always
> the same? And print the address to store prev-next.

This looks exactly the same error I am having with MIDI gadget. In my
case in the udc driver the add_td_to_list() is returning an error
after a dma_pool_alloc() failed. In this case I believe is something
related to f_midi not releasing properly the usb request.

Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: MIDI gadget allocating too much from coherent pool

[Felipe Tonello]

After some debugging, here is where I am:

The crash trace is like this:

(f_midi.c)
-> ALSA calls f_midi_in_trigger()
  ->tasklet_hi_schedule(>tasklet)
-> f_midi_transmit(midi, NULL) the NULL here causes the f_midi to
request a usb_request allocation, also it sets req->complete (which is
not been called? I am investigating this)
  -> usb_ep_queue(ep, req, GFP_ATOMIC)
(chipidea/udc.c)
-> ep_queue(ep, req, gfp_flags)
  -> _ep_queue(ep, req, gfp_flags)
-> _hardware_enqueue(hwep, hwreq) here is where it is crashing
  -> add_td_to_list(hwep, hwreq, count) this guy returns
an error from dma_pool_alloc(), out of mem, which is not been checked
(I have a patch for this)
-> lastnode = list_entry(hwreq->tds.prev, struct
td_node, td) get last node (which is NULL)
  -> lastnode->ptr->next = cpu_to_le32(TD_TERMINATE)
CRASH!!! lastnode->ptr is NULL

My idea is that some how the f_midi is not calling free_ep_req(ep,
req) properly. I am still investigating this.

One thing to bear in mind is that I used ether gadget and mass_storage
gadget as well and they both work just fine. Also that the device I am
running generates a *lot* of MIDI output, much more than the normal
usage, which might be triggering this bug that no one previously had.

Any comments?

Felipe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: MIDI gadget allocating too much from coherent pool

[Felipe Tonello]

Balbi,

On Fri, Sep 18, 2015 at 5:02 PM, Felipe Balbi <ba...@ti.com> wrote:
> Hi,
>
> On Fri, Sep 18, 2015 at 04:53:09PM +0100, Felipe Tonello wrote:
>> Hi Balbi,
>>
>> On Fri, Sep 18, 2015 at 3:25 PM, Felipe Balbi <ba...@ti.com> wrote:
>> > On Fri, Sep 18, 2015 at 11:03:26AM +0100, Felipe Tonello wrote:
>> >> After some debugging, here is where I am:
>> >>
>> >> The crash trace is like this:
>> >>
>> >> (f_midi.c)
>> >> -> ALSA calls f_midi_in_trigger()
>> >>   ->tasklet_hi_schedule(>tasklet)
>> >> -> f_midi_transmit(midi, NULL) the NULL here causes the f_midi to
>> >> request a usb_request allocation, also it sets req->complete (which is
>> >> not been called? I am investigating this)
>> >
>> > ->complete() is called by the UDC when the request actually completes.
>>
>> The problem is that the udc driver is never calling it. I just realised that.
>>
>> (chipidea/udc.c) irqreturn_t udc_irq(struct ci_hdrc *ci) is not been
>> called after a f_midi_transmit(). Think here is the main problem. Any
>> ideas?
>
> have you looked at ci_irq() ? Look at how many return points it has. It's
> not inconceivable that it might be returning before calling ->udc_irq().

Looking into that. Thanks for the hint.

>
> Now, if that's valid or not, it's for you decide. I don't even have this
> HW and can't help much more than this.
>
>> >>   -> usb_ep_queue(ep, req, GFP_ATOMIC)
>> >> (chipidea/udc.c)
>> >> -> ep_queue(ep, req, gfp_flags)
>> >>   -> _ep_queue(ep, req, gfp_flags)
>> >> -> _hardware_enqueue(hwep, hwreq) here is where it is crashing
>> >>   -> add_td_to_list(hwep, hwreq, count) this guy returns
>> >> an error from dma_pool_alloc(), out of mem, which is not been checked
>> >> (I have a patch for this)
>> >
>> > oh, this probably part of the error. Care to send it ?
>>
>> Yes. It doesn't fix anything, but it prevents the kernel to panic.
>
> fair enough, and where's the patch ?

Sent.

>
>> >> -> lastnode = list_entry(hwreq->tds.prev, struct
>> >> td_node, td) get last node (which is NULL)
>> >>   -> lastnode->ptr->next = cpu_to_le32(TD_TERMINATE)
>> >> CRASH!!! lastnode->ptr is NULL
>> >
>> > if lastnode was initialized with list_entry(), it can't be
>> > NULL. You're missing something
>>
>> lastnode is not NULL, lastnode->ptr is NULL. (because of the
>> dma_pool_alloc, check add_td_to_list function).
>
> right, so you're running out of memory. Why are you running out of memory ?
> probably because ->udc_irq() is never called and, thus, you never free from
> coherent. Find reason for that and your problems will be gone, probably.

Bingo! That is what it looks like.

The weird thing, like I said, is that I use different gadgets and they
don't have this leak problem, apparently. I didn't test but it never
panic'ed.

>
>> >> Any comments?
>> >
>> > yeah, continue debugging, what other information can you gather ? Does the
>> > problem go away if you add a ton of printks() around the code ?
>>
>> It doesn't change anything.
>
> ok, then it should be easy to reproduce. Wonder if this UDC has ever been
> tested with isochronous transfers at all.

Perhaps Peter could say better?

Thanks Balbi.

Felipe Tonello
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: MIDI gadget allocating too much from coherent pool

[Felipe Tonello]

Hi Balbi,

On Fri, Sep 18, 2015 at 3:25 PM, Felipe Balbi <ba...@ti.com> wrote:
> On Fri, Sep 18, 2015 at 11:03:26AM +0100, Felipe Tonello wrote:
>> After some debugging, here is where I am:
>>
>> The crash trace is like this:
>>
>> (f_midi.c)
>> -> ALSA calls f_midi_in_trigger()
>>   ->tasklet_hi_schedule(>tasklet)
>> -> f_midi_transmit(midi, NULL) the NULL here causes the f_midi to
>> request a usb_request allocation, also it sets req->complete (which is
>> not been called? I am investigating this)
>
> ->complete() is called by the UDC when the request actually completes.

The problem is that the udc driver is never calling it. I just realised that.

(chipidea/udc.c) irqreturn_t udc_irq(struct ci_hdrc *ci) is not been
called after a f_midi_transmit(). Think here is the main problem. Any
ideas?

>
>>   -> usb_ep_queue(ep, req, GFP_ATOMIC)
>> (chipidea/udc.c)
>> -> ep_queue(ep, req, gfp_flags)
>>   -> _ep_queue(ep, req, gfp_flags)
>> -> _hardware_enqueue(hwep, hwreq) here is where it is crashing
>>   -> add_td_to_list(hwep, hwreq, count) this guy returns
>> an error from dma_pool_alloc(), out of mem, which is not been checked
>> (I have a patch for this)
>
> oh, this probably part of the error. Care to send it ?

Yes. It doesn't fix anything, but it prevents the kernel to panic.

>
>> -> lastnode = list_entry(hwreq->tds.prev, struct
>> td_node, td) get last node (which is NULL)
>>   -> lastnode->ptr->next = cpu_to_le32(TD_TERMINATE)
>> CRASH!!! lastnode->ptr is NULL
>
> if lastnode was initialized with list_entry(), it can't be
> NULL. You're missing something

lastnode is not NULL, lastnode->ptr is NULL. (because of the
dma_pool_alloc, check add_td_to_list function).

>
>> My idea is that some how the f_midi is not calling free_ep_req(ep,
>> req) properly. I am still investigating this.
>>
>> One thing to bear in mind is that I used ether gadget and mass_storage
>> gadget as well and they both work just fine. Also that the device I am
>> running generates a *lot* of MIDI output, much more than the normal
>> usage, which might be triggering this bug that no one previously had.
>>
>> Any comments?
>
> yeah, continue debugging, what other information can you gather ? Does the
> problem go away if you add a ton of printks() around the code ?

It doesn't change anything.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: chipidea: udc: improve error handling on ep_queue

[Felipe Tonello]

Hi Felipe,

On Fri, Sep 18, 2015 at 6:17 PM, Felipe Balbi  wrote:
> Hi,
>
> On Fri, Sep 18, 2015 at 06:12:40PM +0100, e...@felipetonello.com wrote:
>> From: "Felipe F. Tonello" 
>>
>> _ep_queue() didn't check for errors when using add_td_to_list()
>> which can fail if dma_pool_alloc fails, thus causing a kernel
>> panic when lastnode->ptr is NULL.
>>
>> Signed-off-by: Felipe F. Tonello 
>
> this can still be split down further.
>
>> ---
>>  drivers/usb/chipidea/udc.c | 26 +++---
>>  1 file changed, 19 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
>> index 764f668..7169113e 100644
>> --- a/drivers/usb/chipidea/udc.c
>> +++ b/drivers/usb/chipidea/udc.c
>> @@ -404,9 +404,9 @@ static inline u8 _usb_addr(struct ci_hw_ep *ep)
>>  }
>>
>>  /**
>> - * _hardware_queue: configures a request at hardware level
>> - * @gadget: gadget
>> + * _hardware_enqueue: configures a request at hardware level
>>   * @hwep:   endpoint
>> + * @hwreq:  request
>
> this is a cleanup and you shouldn't have a fix depending on a
> cleanup. Fixes are merged during the -rc cycle, while cleanups will be
> deferred to the following merge window.

Got it.

>
>>   *
>>   * This function returns an error code
>>   */
>> @@ -435,19 +435,27 @@ static int _hardware_enqueue(struct ci_hw_ep *hwep, 
>> struct ci_hw_req *hwreq)
>>   if (hwreq->req.dma % PAGE_SIZE)
>>   pages--;
>>
>> - if (rest == 0)
>> - add_td_to_list(hwep, hwreq, 0);
>> + if (rest == 0) {
>> + ret = add_td_to_list(hwep, hwreq, 0);
>> + if (ret < 0)
>> + goto done;
>> + }
>
> this is your fix.
>
>>
>>   while (rest > 0) {
>>   unsigned count = min(hwreq->req.length - hwreq->req.actual,
>>   (unsigned)(pages * CI_HDRC_PAGE_SIZE));
>> - add_td_to_list(hwep, hwreq, count);
>> + ret = add_td_to_list(hwep, hwreq, count);
>> + if (ret < 0)
>> + goto done;
>
> and this
>
>>   rest -= count;
>>   }
>>
>>   if (hwreq->req.zero && hwreq->req.length
>> - && (hwreq->req.length % hwep->ep.maxpacket == 0))
>> - add_td_to_list(hwep, hwreq, 0);
>> + && (hwreq->req.length % hwep->ep.maxpacket == 0)) {
>> + ret = add_td_to_list(hwep, hwreq, 0);
>> + if (ret < 0)
>> + goto done;
>> + }
>>
>
>
> and this.
>
>>   firstnode = list_first_entry(>tds, struct td_node, td);
>>
>> @@ -750,8 +758,12 @@ static void isr_get_status_complete(struct usb_ep *ep, 
>> struct usb_request *req)
>>
>>  /**
>>   * _ep_queue: queues (submits) an I/O request to an endpoint
>> + * @ep:endpoint
>> + * @req:   request
>> + * @gfp_flags: GFP flags (not used)
>
> cleanup
>
>>   *
>>   * Caller must hold lock
>> + * This function returns an error code
>
> somewhat pointless, but could come with the cleanup, no strong
> feelings.
>
>>   */
>>  static int _ep_queue(struct usb_ep *ep, struct usb_request *req,
>>   gfp_t __maybe_unused gfp_flags)
>> --
>> 2.1.4
>>
>
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] usb: gadget: f_midi: check for error on usb_ep_queue

[Felipe Tonello]

On Fri, Sep 18, 2015 at 6:30 PM,   wrote:
> From: "Felipe F. Tonello" 
>
> f_midi is not checking weather the is an error on usb_ep_queue
> request, ignoring potential problems, such as memory leaks.
>
> Signed-off-by: Felipe F. Tonello 
> ---
>
> Changes for v2:
>   - Update code style.
>
>  drivers/usb/gadget/function/f_midi.c | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_midi.c 
> b/drivers/usb/gadget/function/f_midi.c
> index ad50a67..c04133422 100644
> --- a/drivers/usb/gadget/function/f_midi.c
> +++ b/drivers/usb/gadget/function/f_midi.c
> @@ -543,10 +543,16 @@ static void f_midi_transmit(struct f_midi *midi, struct 
> usb_request *req)
> }
> }
>
> -   if (req->length > 0)
> -   usb_ep_queue(ep, req, GFP_ATOMIC);
> -   else
> +   if (req->length > 0) {
> +   int err;
> +
> +   err = usb_ep_queue(ep, req, GFP_ATOMIC);
> +   if (err < 0)
> +   ERROR(midi, "%s queue req: %d\n",
> + midi->out_ep->name, err);

I just realised there is a problem here. It is in_ep in this case. I
will fix it in v3.

> +   } else {
> free_ep_req(ep, req);
> +   }
>  }
>
>  static void f_midi_in_tasklet(unsigned long data)
> --
> 2.1.4
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: MIDI gadget allocating too much from coherent pool

[Felipe Tonello]

Hi Estevam,

On Tue, Sep 15, 2015 at 5:09 AM, Fabio Estevam <feste...@gmail.com> wrote:
> On Thu, Sep 10, 2015 at 6:47 AM, Felipe Tonello <e...@felipetonello.com> 
> wrote:
>> Hi,
>>
>> I have the following setup:
>>
>> DSP (read sensors and read/send MIDI) <- UART -> SOC (imx6) <- USB
>> MIDI GADGET -> HOST
>>
>> When the throughput from the DSP is high, thus causing the throughput
>> on the USB to be high as well, I get a Kernel Panic saying to increase
>> the coherent_pool. I've used some crazy sizes like 1M, 4M and even 8M.
>> Obviously when I increase, it takes longer to crash but it still
>> crashes.
>>
>> I am using linux-fsl 3.14.28.
>
> Please test it with 4.2 or 4.3-rc1 kernel.

I tested on 4.2 and still crashes. Same thing. But it doesn't say
about the dma any longer.

[  148.712916] Unable to handle kernel NULL pointer dereference at
virtual address 
[  148.721135] pgd = 80004000
[  148.723859] [] *pgd=
[  148.727475] Internal error: Oops: 817 [#1] PREEMPT SMP ARM
[  148.732975] Modules linked in: snd_seq_midi snd_seq_dummy
snd_seq_midi_event snd_seq usb_f_midi snd_rawmidi snd_seq_device
g_midi libcomposite configfs snd_soc_fsl_ssi ime
[  148.761694] CPU: 0 PID: 3 Comm: ksoftirqd/0 Not tainted
4.2.0-mx6-seaboard-dirty #1
[  148.769364] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[  148.775909] task: ee070980 ti: ee086000 task.ti: ee086000
[  148.781342] PC is at _ep_queue.isra.18+0x178/0x480
[  148.786150] LR is at 0xeae61480
[  148.789309] pc : [<803c55a4>]lr : []psr: 600f0093
[  148.789309] sp : ee087de0  ip : 6e086000  fp : ee087e1c
[  148.800802] r10: 4000  r9 : ee261010  r8 : eae62234
[  148.806040] r7 :   r6 : 0004  r5 : ee261774  r4 : eae62200
[  148.812579] r3 : eae6223c  r2 :   r1 : 0001  r0 : fff4
[  148.819124] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM
Segment kernel
[  148.826534] Control: 10c5387d  Table: 7cc2004a  DAC: 0015
[  148.832294] Process ksoftirqd/0 (pid: 3, stack limit = 0xee086210)
[  148.838489] Stack: (0xee087de0 to 0xee088000)
[  148.842870] 7de0: 0001 ecdc9f00 a00f0013 eae6223c ee087e24
ee261774 eae62200 a00f0013
[  148.851069] 7e00: eae62200 ed69f000 ed69f134 ed69f134 ee087e3c
ee087e20 803c6780 803c5438
[  148.859268] 7e20: 803c673c   eaef9c00 ee087e7c
ee087e40 7f16d850 803c6748
[  148.867466] 7e40: ee261774 ed69f0f8 8004ed40 4b308e40 ee7adac0
ed69f13c  80761a00
[  148.875664] 7e60: ee087e90 ed69f140 8072524c ee086010 ee087e8c
ee087e80 7f16d8a4 7f16d428
[  148.883861] 7e80: ee087ec4 ee087e90 800290f4 7f16d89c 800446b8
 80761048 
[  148.892059] 7ea0:  8072c080 ee086000 0100 4000
8072c080 ee087f24 ee087ec8
[  148.900257] 7ec0: 80029444 80029078 8072c6fc  04208040
8072c100 c4e7 80545d48
[  148.908455] 7ee0: 000a 80761a00 807252c8 8072c080 ee087ec8
ee086008 ee01fa40 ee086000
[  148.916652] 7f00: ee01fa40 80731b2c   
 ee087f34 ee087f28
[  148.924850] 7f20: 800295a4 8002931c ee087f5c ee087f38 80043314
80029570 ee070980 
[  148.933048] 7f40: ee01fa80 ee01fa40 800431c0  ee087fac
ee087f60 8003ff28 800431cc
[  148.941246] 7f60: 1508a303 0001  ee01fa40 
00030003 ee087f78 ee087f78
[  148.949442] 7f80:   ee087f88 ee087f88 ee01fa80
8003fe44  
[  148.957639] 7fa0:  ee087fb0 8000fb08 8003fe50 
  
[  148.965836] 7fc0:     
  
[  148.974034] 7fe0:     0013
 980f662a 0a9c0780
[  148.982219] Backtrace:
[  148.984714] [<803c542c>] (_ep_queue.isra.18) from [<803c6780>]
(ep_queue+0x44/0x64)
[  148.992382]  r10:ed69f134 r9:ed69f134 r8:ed69f000 r7:eae62200
r6:a00f0013 r5:eae62200
[  149.000319]  r4:ee261774
[  149.002909] [<803c673c>] (ep_queue) from [<7f16d850>]
(f_midi_transmit+0x434/0x474 [usb_f_midi])
[  149.011705]  r6:eaef9c00 r5: r4: r3:803c673c
[  149.017476] [<7f16d41c>] (f_midi_transmit [usb_f_midi]) from
[<7f16d8a4>] (f_midi_in_tasklet+0x14/0x18 [usb_f_midi])
[  149.028009]  r10:ee086010 r9:8072524c r8:ed69f140 r7:ee087e90
r6:80761a00 r5:
[  149.035947]  r4:ed69f13c
[  149.038536] [<7f16d890>] (f_midi_in_tasklet [usb_f_midi]) from
[<800290f4>] (tasklet_hi_action+0x88/0x124)
[  149.048219] [<8002906c>] (tasklet_hi_action) from [<80029444>]
(__do_softirq+0x134/0x254)
[  149.048219] [<8002906c>] (tasklet_hi_action) from [<80029444>]
(__do_softirq+0x134/0x254)
[  149.056407]  r10:8072c080 r9:4000 r8:0100 r7:ee086000
r6:8072c080 r5:
[  149.064343]  r4:
[  149.066917] [<80029310>] (__do_softirq) from [<800295

MIDI gadget allocating too much from coherent pool

[Felipe Tonello]

Hi,

I have the following setup:

DSP (read sensors and read/send MIDI) <- UART -> SOC (imx6) <- USB
MIDI GADGET -> HOST

When the throughput from the DSP is high, thus causing the throughput
on the USB to be high as well, I get a Kernel Panic saying to increase
the coherent_pool. I've used some crazy sizes like 1M, 4M and even 8M.
Obviously when I increase, it takes longer to crash but it still
crashes.

I am using linux-fsl 3.14.28.

The BT as follows:

[  193.619701] ERROR: 1024 KiB atomic DMA coherent pool is too small!
[  193.619701] Please increase it with coherent_pool= kernel parameter!
[  193.632261] Unable to handle kernel NULL pointer dereference at
virtual address 
[  193.640358] pgd = 80004000
[  193.643070] [] *pgd=
[  193.646670] Internal error: Oops: 817 [#1] PREEMPT SMP ARM
[  193.652162] Modules linked in:0d 0 :s0n2d:_5s2e.q2_2m1i
snd_seq_midi_event snd_seq_dummy snd_seq snd_soc_cs4272
snd_soc_fsl_ssi imx_pcm_dma imx_pcm_fiq snd_soc_imx_cs4272
snd_soc_core snd_compress snd_pcm_dmaengine snd_pcm snd_te
 [  193.689447] CPU: 2 PID: 16 Comm: ksoftirqd/2 Not tainted
3.14.28-1.0.0_ga+g91cf351 #1
 [  193.697285] task: ea0a5100 ti: ea0d8000 task.ti: ea0d8000
 [  193.702708] PC is at _ep_queue.isra.24+0x178/0x480
 [  193.707508] LR is at 0xc43cee80
 [  193.710657] pc : [<8039ea38>]lr : []psr: 600f0093
 [  193.710657] sp : ea0d9de0  ip : 6e0ea000  fp : ea0d9e1c
 [  193.722139] r10: 5000  r9 : ea03c010  r8 : c43d05b4
 [  193.727369] r7 :   r6 : 0004  r5 : ea03c6f4  r4 : c43d0580
 [  193.733899] r3 : c43d05bc  r2 :   r1 : 0001  r0 : fff4
 [  193.740433] Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM
Segment kernel
 [  193.747834] Control: 10c5387d  Table: 7aca804a  DAC: 0015
 [  193.753585] Process ksoftirqd/2 (pid: 16, stack limit = 0xea0d8238)
 [  193.759857] Stack: (0xea0d9de0 to 0xea0da000)
 [  193.764225] 9de0: 0001 eaa72480 a00f0013 c43d05bc ea0d9e24
0003 ea03c6f4 a00f0013
 [  193.772413] 9e00: c43d0580 ea51a200 ea51a328 ea51a328 ea0d9e3c
ea0d9e20 8039fc30 8039e8cc
 [  193.780598] 9e20:   c4467000 c43d0580 ea0d9e7c
ea0d9e40 7f078754 8039fbe8
 [  193.788784] 9e40: ea03c6f4 ea51a2ec 80009990 6f00 ea0d9e84
ea51a330 ea51a334 
 [  193.796968] 9e60: 80718f40  ea0d8000 806d82bc ea0d9e8c
ea0d9e80 7f0787a0 7f07832c
 [  193.805153] 9e80: ea0d9ebc ea0d9e90 80031fe4 7f078798 80031f60
  806dc080
 [  193.826207] 9ec0:  ea0d9f0c 04208040 806dc0c0 d672
80507358 000a 80718f40
 [  193.834392] 9ee0: ea0d8000 806d8458 806dc080 0002 80053c8c
ea0d8000 ea0870c0 806e9cc8
 [  193.842576] 9f00: 0001    ea0d9f34
ea0d9f20 80032490 800321f0
 [  193.868396] 9f40: ea087140 ea0870c0 800512e0  ea0d9fac
ea0d9f60 8004a458 800512ec
 [  193.876582] 9f60: 30b80088 0001 0002 ea0870c0 
00030003 ea0d9f78 ea0d9f78
 [  193.884766] 9f80:   ea0d9f88 ea0d9f88 ea087140
8004a384  
 [  193.892952] 9fa0:  ea0d9fb0 8000ea58 8004a390 
  
 [  193.901137] 9fc0:     
  
 [  193.909322] 9fe0:     0013
 2001a1a0 61391094
 [  193.917501] Backtrace:
 [  193.919982] [<8039e8c0>] (_ep_queue.isra.24) from [<8039fc30>]
(ep_queue+0x54/0x88)
 [  193.927643]  r10:ea51a328 r9:ea51a328 r8:ea51a200 r7:c43d0580
r6:a00f0013 r5:ea03c6f4
 [  193.935554]  r4:0003
 [  193.938120] [<8039fbdc>] (ep_queue) from [<7f078754>]
(f_midi_transmit+0x434/0x46c [g_midi])
 [  193.946562]  r7:c43d0580 r6:c4467000 r5: r4:
 [  193.952299] [<7f078320>] (f_midi_transmit [g_midi]) from
[<7f0787a0>] (f_midi_in_tasklet+0x14/0x18 [g_midi])
 [  193.962129]  r10:806d82bc r9:ea0d8000 r8: r7:80718f40
r6: r5:ea51a334
 [  193.970039]  r4:ea51a330
 [  193.972609] [<7f07878c>] (f_midi_in_tasklet [g_midi]) from
[<80031fe4>] (tasklet_hi_action+0x84/0x114)
 [  193.981928] [<80031f60>] (tasklet_hi_action) from [<8003232c>]
(__do_softirq+0x148/0x260)
 [  193.990107]  r10:4000 r9:806dc080 r8:0100 r7:ea0d8000
r6:806dc080 r5:
 [  193.998018]  r4: r3:80031f60
 [  194.001635] [<800321e4>] (__do_softirq) from [<80032490>]
(run_ksoftirqd+0x4c/0x64)
 [  194.009294]  r10: r9: r8: r7:0001
r6:806e9cc8 r5:ea0870c0
 [  194.017203]  r4:ea0d8000
 [  194.019773] [<80032444>] (run_ksoftirqd) from [<80051468>]
(smpboot_thread_fn+0x188/0x278)
 [  194.028059] [<800512e0>] (smpboot_thread_fn) from [<8004a458>]
(kthread+0xd4/0xec)
 [  194.035632]  r8: r7:800512e0 r6:ea0870c0 r5:ea087140
r4: r3:ea0a5100
 [  194.043468] [<8004a384>] (kthread) from [<8000ea58>]
(ret_from_fork+0x14/0x3c)
 [  194.050695]  r7: r6: r5:8004a384 r4:ea087140
 [  194.056426] Code: e3a01001 e594203c e50b2030 e593200c (e5821000)
 [  194.062529] ---[ end