Re: [PATCH 03/23] usb: gadget: f_sourcesink: free requests in sourcesink_disable()

[Krzysztof Opasiak]

Hi,

On 11/10/2015 03:02 AM, Peter Chen wrote:
> On Fri, Nov 06, 2015 at 10:58:39AM +0100, Krzysztof Opasiak wrote:
> > On 11/06/2015 10:48 AM, Peter Chen wrote:
> > > On Fri, Nov 06, 2015 at 09:50:11AM +0100, Robert Baldyga wrote:
> > > > On 11/06/2015 09:15 AM, Peter Chen wrote:
> > > > > On Tue, Nov 03, 2015 at 01:53:42PM +0100, Robert Baldyga wrote:
> > > > > > USB requests in SourceSink function are allocated in sourcesink_get_alt()
> > > > > > function, so we prefer to free them rather in sourcesink_disable() than
> > > > > > in source_sink_complete() when request is completed with error. It provides
> > > > > > better symetry in resource management and improves code readability.
> > > > > >
> > > > > > Signed-off-by: Robert Baldyga <r.baldyga@xxxxxxxxxxx>
> > > > > > ---
> > > > > >   drivers/usb/gadget/function/f_sourcesink.c | 33 +++++++++++++++++++++++++++---
> > > > > >   1 file changed, 30 insertions(+), 3 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/usb/gadget/function/f_sourcesink.c b/drivers/usb/gadget/function/f_sourcesink.c
> > > > > > index a8b68c6..d8f5f56 100644
> > > > > > --- a/drivers/usb/gadget/function/f_sourcesink.c
> > > > > > +++ b/drivers/usb/gadget/function/f_sourcesink.c
> > > > > > @@ -22,6 +22,8 @@
> > > > > >   #include "g_zero.h"
> > > > > >   #include "u_f.h"
> > > > > >
> > > > > > +#define REQ_CNT	8
> > > > > > +
> > > > >
> > > > > It would be buffer if we can have module parameter for this like
> > > > > qlen at f_loopback.
> > > > > > @@ -561,7 +568,6 @@ static void source_sink_complete(struct usb_ep *ep, struct usb_request *req)
> > > > > >   				req->actual, req->length);
> > > > > >   		if (ep == ss->out_ep)
> > > > > >   			check_read_data(ss, req);
> > > > > > -		free_ep_req(ep, req);
> > > > >
> > > > > I find the current code may cause memory leak, since above code
> > > > > is only called one time.
> > > >
> > > > I don't see what you mean. Can you explain a bit more in which situation
> > > > can memory leak take place?
> > >
> > > Oh, sorry, I have not seen there is a "break;" at the source_sink_start_ep for
> > > none-iso endpoint, so, I am wrong, no memory leak here.
> > >
> > > I have tried changing bulk for 8 requests, the performance improves
> > > greatly, but still a little problem for OUT,  I will see what's the matter.
> > > Besides, it will be better we can have a qlen parameters like f_loopback,
> > > in that case, we can improve the performance for gadget, and get the best performance
> > > when testing with usbtest, I will do it later.
> >
> > Moreover, I would suggest to add qlen and iso_qlen params not only
> > qlen as even now we are using different qlen for bulk and iso.
>
> Krzysztof, would you know why we have different qlen for bulk and iso now?

Well, in my opinion iso and bulk use different buf sizes and simply 
different number of requests is needed to saturate the bandwidth.

--
Krzysztof Opasiak
Samsung R&D Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/