Hello Lukasz,
On 2018년 05월 25일 07:52, Lukasz Majewski wrote:
Hi Seung-Woo,
There are memory leaks of usb request and its buffer for ep0, in_ep, and out ep. Fix memory leaks of usb request and its buffer.
Signed-off-by: Seung-Woo Kim sw0312.kim@samsung.com
Change from v1
- remove allocation of out_ep request instead of allocating and
freeing
- fix use error path instead of duplicated error handling code
drivers/usb/gadget/f_thor.c | 45 ++++++++++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 16 deletions(-)
diff --git a/drivers/usb/gadget/f_thor.c b/drivers/usb/gadget/f_thor.c index c8eda05..02d6844 100644 --- a/drivers/usb/gadget/f_thor.c +++ b/drivers/usb/gadget/f_thor.c @@ -752,6 +752,13 @@ int thor_handle(void) return 0; }
+static void free_ep_req(struct usb_ep *ep, struct usb_request *req) +{
- if (req->buf)
free(req->buf);- usb_ep_free_request(ep, req);
+}
static int thor_func_bind(struct usb_configuration *c, struct usb_function *f) { struct usb_gadget *gadget = c->cdev->gadget; @@ -860,21 +867,18 @@ static int thor_func_bind(struct usb_configuration *c, struct usb_function *f) return 0;
fail:
- if (dev->req)
free_ep_req(gadget->ep0, dev->req);}
-static void free_ep_req(struct usb_ep *ep, struct usb_request *req) -{
- free(req->buf);
- usb_ep_free_request(ep, req);
-}
static void thor_unbind(struct usb_configuration *c, struct usb_function *f) { struct f_thor *f_thor = func_to_thor(f); struct thor_dev *dev = f_thor->dev;
- free_ep_req(dev->gadget->ep0, dev->req);
Till this change - no issues.
free(dev); memset(thor_func, 0, sizeof(*thor_func)); thor_func = NULL; @@ -895,8 +899,6 @@ static void thor_func_disable(struct usb_function *f) }
if (dev->out_ep->driver_data) {
free(dev->out_req->buf);dev->out_req->buf = NULL;I think that this setting (to NULL) was needed to be able to ctrl+C from thor command and then run it again (as some code checks if buf is NULL).
From the comment about usb_ep_free_request(), it frees request object.
So, it looks not required. Actually, dev->out_req = NULL; is more necessary, but in my test, ctrl-c or thor communication failure also flow till thor_unbind() where dev is also freed.
usb_ep_free_request(dev->out_ep, dev->out_req); usb_ep_disable(dev->out_ep); dev->out_ep->driver_data = NULL;@@ -924,14 +926,13 @@ static int thor_eps_setup(struct usb_function *f) result = usb_ep_enable(ep, d); if (result)
goto exit;
goto err;ep->driver_data = cdev; /* claim */ req = thor_start_ep(ep); if (!req) {
usb_ep_disable(ep);goto exit;
goto err_disable_in_ep;}
dev->in_req = req;
@@ -941,22 +942,34 @@ static int thor_eps_setup(struct usb_function *f) result = usb_ep_enable(ep, d); if (result)
goto exit;
goto err_free_in_req;ep->driver_data = cdev; /* claim */
- req = thor_start_ep(ep);
- req = usb_ep_alloc_request(ep, 0);
Is this safe to replace thor_start_ep() - which tunes the ep params - with generic function?
It is safe, because there is no tuning ep param. The function has 3 steps including usb_ep_alloc_request() and allocating buffer with memalign() and setting complete() callback to thor_rx_tx_complete(). For out_req, buffer allocation is not required because buffer for out_req is always set from thor_set_dma() usually with dfu_buffer before rx.
( I do see the req->complete = thor_rx_tx_complete below ).
If the thor_start_ep can be replaced with generic code, then maybe we can remove it?
It is possible to replace in_req case. If you prefer that, I will send v3 after replacing thor_start_ep() usage with generic functions.
Best Regards, - Seung-Woo Kim
if (!req) {
usb_ep_disable(ep);goto exit;
goto err_disable_out_ep;}
req->complete = thor_rx_tx_complete; dev->out_req = req; /* ACM control EP */ ep = dev->int_ep; ep->driver_data = cdev; /* claim */
- exit:
- return 0;
- err_disable_out_ep:
- usb_ep_disable(dev->out_ep);
- err_free_in_req:
- free_ep_req(dev->in_ep, dev->in_req);
- dev->in_req = NULL;
- err_disable_in_ep:
- usb_ep_disable(dev->in_ep);
- err: return result;
}
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de