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. Peter > > I've only changed place where requests are freed. Now they are freed in > sourcesink_disable(). The current code is even more memory leak > resistant, because requests will be freed even in case of failure of > usb_ep_queue() in source_sink_complete(), which was not provided so far. > > >> return; > >> > >> case -EOVERFLOW: /* buffer overrun on read means that > >> @@ -613,7 +619,7 @@ static int source_sink_start_ep(struct f_sourcesink *ss, bool is_in, > >> ep = is_in ? ss->in_ep : ss->out_ep; > >> } > >> > >> - for (i = 0; i < 8; i++) { > >> + for (i = 0; i < REQ_CNT; i++) { > >> req = ss_alloc_ep_req(ep, size); > >> if (!req) > >> return -ENOMEM; > >> @@ -635,8 +641,18 @@ static int source_sink_start_ep(struct f_sourcesink *ss, bool is_in, > >> free_ep_req(ep, req); > >> } > >> > >> - if (!is_iso) > >> + if (is_iso) { > >> + if (is_in) > >> + ss->iso_in_req[i] = req; > >> + else > >> + ss->iso_out_req[i] = req; > >> + } else { > >> + if (is_in) > >> + ss->in_req = req; > >> + else > >> + ss->out_req = req; > > > > The req will be different for both bulk and iso, why you only > > have array for iso requests? > > Because only for iso endpoints there is allocated more than one request. > I don't know why, but I've decided to not change this. > > > > >> break; > >> + } > >> } > >> > >> return status; > >> @@ -764,8 +780,19 @@ static int sourcesink_get_alt(struct usb_function *f, unsigned intf) > >> static void sourcesink_disable(struct usb_function *f) > >> { > >> struct f_sourcesink *ss = func_to_ss(f); > >> + int i; > >> > >> disable_source_sink(ss); > >> + > >> + free_ep_req(ss->in_ep, ss->in_req); > >> + free_ep_req(ss->out_ep, ss->out_req); > >> + > >> + if (ss->iso_in_ep) { > >> + for (i = 0; i < REQ_CNT; i++) { > >> + free_ep_req(ss->iso_in_ep, ss->iso_in_req[i]); > >> + free_ep_req(ss->iso_out_ep, ss->iso_out_req[i]); > >> + } > >> + } > >> } > >> > >> /*-------------------------------------------------------------------------*/ > >> -- > >> 1.9.1 > >> > >> -- > >> To unsubscribe from this list: send the line "unsubscribe linux-usb" in > >> the body of a message to majordomo@xxxxxxxxxxxxxxx > >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html