Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757355AbbKFIuj (ORCPT ); Fri, 6 Nov 2015 03:50:39 -0500 Received: from mailout4.w1.samsung.com ([210.118.77.14]:40209 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757325AbbKFIuQ (ORCPT ); Fri, 6 Nov 2015 03:50:16 -0500 X-AuditID: cbfec7f5-f794b6d000001495-f2-563c69c5a900 Subject: Re: [PATCH 03/23] usb: gadget: f_sourcesink: free requests in sourcesink_disable() To: Peter Chen References: <1446555242-3733-1-git-send-email-r.baldyga@samsung.com> <1446555242-3733-4-git-send-email-r.baldyga@samsung.com> <20151106081512.GD12560@shlinux2> Cc: balbi@ti.com, gregkh@linuxfoundation.org, andrzej.p@samsung.com, m.szyprowski@samsung.com, b.zolnierkie@samsung.com, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org From: Robert Baldyga Message-id: <563C69C3.2030102@samsung.com> Date: Fri, 06 Nov 2015 09:50:11 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-version: 1.0 In-reply-to: <20151106081512.GD12560@shlinux2> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrHLMWRmVeSWpSXmKPExsVy+t/xy7pHM23CDK41qVrMetnOYrFxxnpW i4P36y2aF69ns7i8aw6bxaJlrcwWa4/cZbc4NvsvkwOHx7/D/Uwe++euYffo27KK0eP4je1M Hp83yQWwRnHZpKTmZJalFunbJXBlnD38kbGgUbFiwyGxBsYuqS5GTg4JAROJTY232SBsMYkL 99YD2VwcQgJLGSVmvt7ABJIQEnjGKNH8URDEFhaIlZhycg87iC0ioCmx7vwxJoiGxYwSd9av AnOYBbYwSqy/f40FpIpNQEdiy/cJjCA2r4CWxOULj8BsFgFViZsbtrGC2KICERITJzSwQtQI SvyYfA+sl1NAT+LPnFVA9RxAQ/Uk7l/UAgkzC8hLbF7zlnkCo8AsJB2zEKpmIalawMi8ilE0 tTS5oDgpPddIrzgxt7g0L10vOT93EyMk1L/uYFx6zOoQowAHoxIPr8Fy6zAh1sSy4srcQ4wS HMxKIrxyzDZhQrwpiZVVqUX58UWlOanFhxilOViUxHln7nofIiSQnliSmp2aWpBaBJNl4uCU amB0UL76SXiDes3sj3s+vz6V/3SCx/wKrwNR/WIVj140qIh/bVvRYX2lo+FBzjMzC/MMAcEv 8VGx1fenx39nUZVgX8vk2XH4YmuPcsLJUwnOQaW9P4/HrJb9suae2bun3n1bpBi6evOckwr7 b7ZFJ2pO/7Ms9/yCfv69NuYTdWd+KrELvLhC+4ESS3FGoqEWc1FxIgDe1d4PcQIAAA== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4342 Lines: 133 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 >> --- >> 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. > >> /* >> * SOURCE/SINK FUNCTION ... a primary testing vehicle for USB peripheral >> * controller drivers. >> @@ -51,6 +53,11 @@ struct f_sourcesink { >> struct usb_ep *iso_out_ep; >> int cur_alt; >> >> + struct usb_request *in_req; >> + struct usb_request *out_req; >> + struct usb_request *iso_in_req[REQ_CNT]; >> + struct usb_request *iso_out_req[REQ_CNT]; >> + >> unsigned pattern; >> unsigned isoc_interval; >> unsigned isoc_maxpacket; >> @@ -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? 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@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- 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/