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

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

[Felipe F. Tonello]

If CONFIG_PM or CONFIG_PM_SLEEP is not set, driver will not compile
properly.

Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
---
 drivers/usb/chipidea/core.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 3ad48e1..4182549 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -1009,18 +1009,22 @@ static int ci_runtime_resume(struct device *dev)
 	return ci_controller_resume(dev);
 }
 
-#endif /* CONFIG_PM */
 static const struct dev_pm_ops ci_pm_ops = {
+#ifdef CONFIG_PM_SLEEP
 	SET_SYSTEM_SLEEP_PM_OPS(ci_suspend, ci_resume)
+#endif /* CONFIG_PM_SLEEP */
 	SET_RUNTIME_PM_OPS(ci_runtime_suspend, ci_runtime_resume, NULL)
 };
+#endif /* CONFIG_PM */
 
 static struct platform_driver ci_hdrc_driver = {
 	.probe	= ci_hdrc_probe,
 	.remove	= ci_hdrc_remove,
 	.driver	= {
 		.name	= "ci_hdrc",
+#ifdef CONFIG_PM
 		.pm	= &ci_pm_ops,
+#endif
 	},
 };
 
-- 
2.1.4

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

[Felipe F. Tonello]

req->actual == req->length means that there is no data left to enqueue,
so free the request.

Signed-off-by: Felipe F. Tonello <eu@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;
 		}
 		break;
-- 
2.1.4

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1139 bytes --]

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 <eu@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);

I'd have to have a deeper look at f_midi, but this doesn't look
correct to me. Why do you think that we should stop queueing
requests when we transfer one in full ?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Peter Chen]

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 <eu@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?

-- 

Best Regards,
Peter Chen

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 <peter.chen@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 <eu@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?

Felipe

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

[Alan Stern]

On Wed, 23 Sep 2015, Felipe Tonello wrote:

> Hi Peter,
> 
> On Wed, Sep 23, 2015 at 4:10 AM, Peter Chen <peter.chen@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 <eu@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?

Don't bother to answer if this doesn't make any sense...

Alan Stern

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 <stern@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.chen@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 <eu@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

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

[Felipe F. Tonello]

This fix a memory leak that will occur in this case.

Signed-off-by: Felipe F. Tonello <eu@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

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 996 bytes --]

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 <eu@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);

sure this request won't be needed on the next time the tasklet is queued ?

Although, this looks correct, let's try to make sure really is correct.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Peter Chen]

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 <eu@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.

-- 

Best Regards,
Peter Chen

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 <peter.chen@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 <eu@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.

Felipe

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

[Peter Chen]

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.chen@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 <eu@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?

-- 

Best Regards,
Peter Chen

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.chen@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.chen@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 <eu@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

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

[Peter Chen]

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.chen@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.chen@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 <eu@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.

-- 

Best Regards,
Peter Chen

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.chen@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.chen@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.chen@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 <eu@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

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1325 bytes --]

Hi,

On Tue, Sep 22, 2015 at 07:59:08PM +0100, Felipe F. Tonello wrote:
> If CONFIG_PM or CONFIG_PM_SLEEP is not set, driver will not compile
> properly.
> 
> Signed-off-by: Felipe F. Tonello <eu@felipetonello.com>
> ---
>  drivers/usb/chipidea/core.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 3ad48e1..4182549 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -1009,18 +1009,22 @@ static int ci_runtime_resume(struct device *dev)
>  	return ci_controller_resume(dev);
>  }
>  
> -#endif /* CONFIG_PM */
>  static const struct dev_pm_ops ci_pm_ops = {
> +#ifdef CONFIG_PM_SLEEP
>  	SET_SYSTEM_SLEEP_PM_OPS(ci_suspend, ci_resume)
> +#endif /* CONFIG_PM_SLEEP */

NAK, this is not the right way to do it. Look at the definition of
SET_SYSTEM_SLEEP_PM_OPS() and other users in the kernel.

>  	SET_RUNTIME_PM_OPS(ci_runtime_suspend, ci_runtime_resume, NULL)
>  };
> +#endif /* CONFIG_PM */
>  
>  static struct platform_driver ci_hdrc_driver = {
>  	.probe	= ci_hdrc_probe,
>  	.remove	= ci_hdrc_remove,
>  	.driver	= {
>  		.name	= "ci_hdrc",
> +#ifdef CONFIG_PM
>  		.pm	= &ci_pm_ops,
> +#endif
>  	},
>  };
>  
> -- 
> 2.1.4
> 

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]