Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964989AbbKDKP5 (ORCPT ); Wed, 4 Nov 2015 05:15:57 -0500 Received: from mail-wm0-f48.google.com ([74.125.82.48]:35593 "EHLO mail-wm0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932586AbbKDKPz (ORCPT ); Wed, 4 Nov 2015 05:15:55 -0500 Subject: Re: [PATCH 04/23] usb: gadget: f_loopback: free requests in loopback_disable() To: Robert Baldyga , balbi@ti.com References: <1446555242-3733-1-git-send-email-r.baldyga@samsung.com> <1446555242-3733-5-git-send-email-r.baldyga@samsung.com> Cc: 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: Felipe Ferreri Tonello Message-ID: <5639DAD8.40204@felipetonello.com> Date: Wed, 4 Nov 2015 10:15:52 +0000 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: <1446555242-3733-5-git-send-email-r.baldyga@samsung.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5472 Lines: 164 Hi Robert, On 03/11/15 12:53, Robert Baldyga wrote: > USB requests in Loopback function are allocated in loopback_get_alt() > function, so we prefer to free them rather in loopback_disable() than > in loopback_complete() when request is completed with error. It provides > better symetry in resource management and improves code readability. Are we doing this for all functions? Because I see that several functions does the same thing, they free IN/OUT ep requests on complete() instead of disable(). I also prefer this, but then we need to refactor most function code. > > Signed-off-by: Robert Baldyga > --- > drivers/usb/gadget/function/f_loopback.c | 58 +++++++++++++------------------- > 1 file changed, 23 insertions(+), 35 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_loopback.c b/drivers/usb/gadget/function/f_loopback.c > index 6b2102b..41464ae 100644 > --- a/drivers/usb/gadget/function/f_loopback.c > +++ b/drivers/usb/gadget/function/f_loopback.c > @@ -35,6 +35,9 @@ struct f_loopback { > struct usb_ep *in_ep; > struct usb_ep *out_ep; > > + struct usb_request *in_req; > + struct usb_request *out_req; > + > unsigned qlen; > unsigned buflen; > }; > @@ -249,30 +252,25 @@ static void loopback_complete(struct usb_ep *ep, struct usb_request *req) > * We received some data from the host so let's > * queue it so host can read the from our in ep > */ > - struct usb_request *in_req = req->context; > - > - in_req->zero = (req->actual < req->length); > - in_req->length = req->actual; > + loop->in_req->zero = (req->actual < req->length); > + loop->in_req->length = req->actual; > + req = loop->in_req; > ep = loop->in_ep; > - req = in_req; > } else { > /* > * We have just looped back a bunch of data > * to host. Now let's wait for some more data. > */ > - req = req->context; > + req = loop->out_req; > ep = loop->out_ep; > } > > /* queue the buffer back to host or for next bunch of data */ > status = usb_ep_queue(ep, req, GFP_ATOMIC); > - if (status == 0) { > - return; > - } else { > + if (status < 0) > ERROR(cdev, "Unable to loop back buffer to %s: %d\n", > ep->name, status); > - goto free_req; > - } > + break; > > /* "should never get here" */ > default: > @@ -280,20 +278,10 @@ static void loopback_complete(struct usb_ep *ep, struct usb_request *req) > status, req->actual, req->length); > /* FALLTHROUGH */ > > - /* NOTE: since this driver doesn't maintain an explicit record > - * of requests it submitted (just maintains qlen count), we > - * rely on the hardware driver to clean up on disconnect or > - * endpoint disable. > - */ > case -ECONNABORTED: /* hardware forced ep reset */ > case -ECONNRESET: /* request dequeued */ > case -ESHUTDOWN: /* disconnect from host */ > -free_req: > - usb_ep_free_request(ep == loop->in_ep ? > - loop->out_ep : loop->in_ep, > - req->context); > - free_ep_req(ep, req); > - return; > + break; > } > } > > @@ -316,7 +304,6 @@ static inline struct usb_request *lb_alloc_ep_req(struct usb_ep *ep, int len) > static int alloc_requests(struct usb_composite_dev *cdev, > struct f_loopback *loop) > { > - struct usb_request *in_req, *out_req; > int i; > int result = 0; > > @@ -329,23 +316,21 @@ static int alloc_requests(struct usb_composite_dev *cdev, > for (i = 0; i < loop->qlen && result == 0; i++) { > result = -ENOMEM; > > - in_req = usb_ep_alloc_request(loop->in_ep, GFP_KERNEL); > - if (!in_req) > + loop->in_req = usb_ep_alloc_request(loop->in_ep, GFP_KERNEL); > + if (!loop->in_req) > goto fail; > > - out_req = lb_alloc_ep_req(loop->out_ep, 0); > - if (!out_req) > + loop->out_req = lb_alloc_ep_req(loop->out_ep, 0); > + if (!loop->out_req) > goto fail_in; > > - in_req->complete = loopback_complete; > - out_req->complete = loopback_complete; > + loop->in_req->complete = loopback_complete; > + loop->out_req->complete = loopback_complete; > > - in_req->buf = out_req->buf; > + loop->in_req->buf = loop->out_req->buf; > /* length will be set in complete routine */ > - in_req->context = out_req; > - out_req->context = in_req; > > - result = usb_ep_queue(loop->out_ep, out_req, GFP_ATOMIC); > + result = usb_ep_queue(loop->out_ep, loop->out_req, GFP_ATOMIC); > if (result) { > ERROR(cdev, "%s queue req --> %d\n", > loop->out_ep->name, result); > @@ -356,9 +341,9 @@ static int alloc_requests(struct usb_composite_dev *cdev, > return 0; > > fail_out: > - free_ep_req(loop->out_ep, out_req); > + free_ep_req(loop->out_ep, loop->out_req); > fail_in: > - usb_ep_free_request(loop->in_ep, in_req); > + usb_ep_free_request(loop->in_ep, loop->in_req); > fail: > return result; > } > @@ -426,6 +411,9 @@ static void loopback_disable(struct usb_function *f) > struct f_loopback *loop = func_to_loop(f); > > disable_loopback(loop); > + > + free_ep_req(loop->out_ep, loop->out_req); > + usb_ep_free_request(loop->in_ep, loop->in_req); > } > > static struct usb_function *loopback_alloc(struct usb_function_instance *fi) > -- 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/