Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1230776 > unrolled thread

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

Started by"Felipe F. Tonello" <eu@felipetonello.com>
First post2015-09-22 21:00 +0200
Last post2015-09-25 12:30 +0200
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 3/3] usb: gadget: f_midi: free request when usb_ep_queue fails "Felipe F. Tonello" <eu@felipetonello.com> - 2015-09-22 21:00 +0200
    Re: [PATCH 3/3] usb: gadget: f_midi: free request when usb_ep_queue  fails Felipe Balbi <balbi@ti.com> - 2015-09-22 23:20 +0200
    Re: [PATCH 3/3] usb: gadget: f_midi: free request when usb_ep_queue fails Felipe Tonello <eu@felipetonello.com> - 2015-09-23 13:50 +0200
      Re: [PATCH 3/3] usb: gadget: f_midi: free request when usb_ep_queue fails Felipe Tonello <eu@felipetonello.com> - 2015-09-25 10:30 +0200
        Re: [PATCH 3/3] usb: gadget: f_midi: free request when usb_ep_queue fails Felipe Tonello <eu@felipetonello.com> - 2015-09-25 12:30 +0200

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

From"Felipe F. Tonello" <eu@felipetonello.com>
Date2015-09-22 21:00 +0200
Subject[PATCH 3/3] usb: gadget: f_midi: free request when usb_ep_queue fails
Message-ID<qbAXh-5tC-47@gated-at.bofh.it>
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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

FromFelipe Balbi <balbi@ti.com>
Date2015-09-22 23:20 +0200
SubjectRe: [PATCH 3/3] usb: gadget: f_midi: free request when usb_ep_queue fails
Message-ID<qbD8K-mn-9@gated-at.bofh.it>
In reply to#1230776

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1231378

FromFelipe Tonello <eu@felipetonello.com>
Date2015-09-23 13:50 +0200
Message-ID<qbQIG-33j-25@gated-at.bofh.it>
In reply to#1230776
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232657

FromFelipe Tonello <eu@felipetonello.com>
Date2015-09-25 10:30 +0200
Message-ID<qcwyf-4jx-23@gated-at.bofh.it>
In reply to#1231378
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232705

FromFelipe Tonello <eu@felipetonello.com>
Date2015-09-25 12:30 +0200
Message-ID<qcyql-6Zt-3@gated-at.bofh.it>
In reply to#1232657
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web