* [U-Boot] [PATCH] gadget: f_thor: Fix memory leaks of usb request and its buffer [not found] <CGME20180523072552epcas1p3b1bdbc7e0f49053fe3ce15117e61b429@epcas1p3.samsung.com> @ 2018-05-23 7:25 ` Seung-Woo Kim 2018-05-24 2:28 ` [U-Boot] [PATCH v2] " Seung-Woo Kim 0 siblings, 1 reply; 5+ messages in thread From: Seung-Woo Kim @ 2018-05-23 7:25 UTC (permalink / raw) To: u-boot 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> --- drivers/usb/gadget/f_thor.c | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-) diff --git a/drivers/usb/gadget/f_thor.c b/drivers/usb/gadget/f_thor.c index c8eda05..ec8bd50 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); free(dev); return status; } -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); free(dev); memset(thor_func, 0, sizeof(*thor_func)); thor_func = NULL; @@ -895,9 +899,9 @@ static void thor_func_disable(struct usb_function *f) } if (dev->out_ep->driver_data) { - free(dev->out_req->buf); + /* buf of out_req is set with thor_set_dma(), so just clear */ dev->out_req->buf = NULL; - usb_ep_free_request(dev->out_ep, dev->out_req); + free_ep_req(dev->out_ep, dev->out_req); usb_ep_disable(dev->out_ep); dev->out_ep->driver_data = NULL; } @@ -940,17 +944,29 @@ static int thor_eps_setup(struct usb_function *f) debug("(d)bEndpointAddress: 0x%x\n", d->bEndpointAddress); result = usb_ep_enable(ep, d); - if (result) + if (result) { + free_ep_req(dev->in_ep, dev->in_req); + dev->in_req = NULL; + usb_ep_disable(dev->in_ep); goto exit; + } ep->driver_data = cdev; /* claim */ req = thor_start_ep(ep); if (!req) { usb_ep_disable(ep); + free_ep_req(dev->in_ep, dev->in_req); + dev->in_req = NULL; + usb_ep_disable(dev->in_ep); result = -EIO; goto exit; } + /* buf of out_req will be set with thor_set_dma(), so clear it */ + free(req->buf); + req->buf = NULL; + req->length = 0; + dev->out_req = req; /* ACM control EP */ ep = dev->int_ep; -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v2] gadget: f_thor: Fix memory leaks of usb request and its buffer 2018-05-23 7:25 ` [U-Boot] [PATCH] gadget: f_thor: Fix memory leaks of usb request and its buffer Seung-Woo Kim @ 2018-05-24 2:28 ` Seung-Woo Kim 2018-05-24 22:52 ` Lukasz Majewski 2018-05-25 5:21 ` [U-Boot] [PATCH v3] " Seung-Woo Kim 0 siblings, 2 replies; 5+ messages in thread From: Seung-Woo Kim @ 2018-05-24 2:28 UTC (permalink / raw) To: u-boot 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); free(dev); return status; } -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); 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; 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); result = -EIO; - 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); if (!req) { - usb_ep_disable(ep); result = -EIO; - 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; } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v2] gadget: f_thor: Fix memory leaks of usb request and its buffer 2018-05-24 2:28 ` [U-Boot] [PATCH v2] " Seung-Woo Kim @ 2018-05-24 22:52 ` Lukasz Majewski 2018-05-25 2:14 ` Seung-Woo Kim 2018-05-25 5:21 ` [U-Boot] [PATCH v3] " Seung-Woo Kim 1 sibling, 1 reply; 5+ messages in thread From: Lukasz Majewski @ 2018-05-24 22:52 UTC (permalink / raw) To: u-boot 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); > free(dev); > return status; > } > > -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). > 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); > result = -EIO; > - 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? ( 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? > if (!req) { > - usb_ep_disable(ep); > result = -EIO; > - 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 at denx.de -------------- next part -------------- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 488 bytes Desc: OpenPGP digital signature URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180525/618f82d7/attachment.sig> ^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v2] gadget: f_thor: Fix memory leaks of usb request and its buffer 2018-05-24 22:52 ` Lukasz Majewski @ 2018-05-25 2:14 ` Seung-Woo Kim 0 siblings, 0 replies; 5+ messages in thread From: Seung-Woo Kim @ 2018-05-25 2:14 UTC (permalink / raw) To: u-boot 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); >> free(dev); >> return status; >> } >> >> -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); >> result = -EIO; >> - 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); >> result = -EIO; >> - 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 at denx.de > -- Seung-Woo Kim Samsung Research -- ^ permalink raw reply [flat|nested] 5+ messages in thread
* [U-Boot] [PATCH v3] gadget: f_thor: Fix memory leaks of usb request and its buffer 2018-05-24 2:28 ` [U-Boot] [PATCH v2] " Seung-Woo Kim 2018-05-24 22:52 ` Lukasz Majewski @ 2018-05-25 5:21 ` Seung-Woo Kim 1 sibling, 0 replies; 5+ messages in thread From: Seung-Woo Kim @ 2018-05-25 5:21 UTC (permalink / raw) To: u-boot 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 v2 - replace only once used local function, thor_start_ep(), with generic functions as Lukasz commented 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 | 65 +++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 33 deletions(-) diff --git a/drivers/usb/gadget/f_thor.c b/drivers/usb/gadget/f_thor.c index c8eda05..1aa6be4 100644 --- a/drivers/usb/gadget/f_thor.c +++ b/drivers/usb/gadget/f_thor.c @@ -620,22 +620,6 @@ static void thor_rx_tx_complete(struct usb_ep *ep, struct usb_request *req) status, req->actual, req->length); } -static struct usb_request *thor_start_ep(struct usb_ep *ep) -{ - struct usb_request *req; - - req = alloc_ep_req(ep, THOR_PACKET_SIZE); - debug("%s: ep:%p req:%p\n", __func__, ep, req); - - if (!req) - return NULL; - - memset(req->buf, 0, req->length); - req->complete = thor_rx_tx_complete; - - return req; -} - static void thor_setup_complete(struct usb_ep *ep, struct usb_request *req) { if (req->status || req->actual != req->length) @@ -752,6 +736,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 +851,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); free(dev); return status; } -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); free(dev); memset(thor_func, 0, sizeof(*thor_func)); thor_func = NULL; @@ -895,8 +883,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; usb_ep_free_request(dev->out_ep, dev->out_req); usb_ep_disable(dev->out_ep); dev->out_ep->driver_data = NULL; @@ -924,16 +910,17 @@ 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); + req = alloc_ep_req(ep, THOR_PACKET_SIZE); if (!req) { - usb_ep_disable(ep); result = -EIO; - goto exit; + goto err_disable_in_ep; } + memset(req->buf, 0, req->length); + req->complete = thor_rx_tx_complete; dev->in_req = req; ep = dev->out_ep; d = ep_desc(gadget, &hs_out_desc, &fs_out_desc); @@ -941,22 +928,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); if (!req) { - usb_ep_disable(ep); result = -EIO; - 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; } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-05-25 5:21 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20180523072552epcas1p3b1bdbc7e0f49053fe3ce15117e61b429@epcas1p3.samsung.com>
2018-05-23 7:25 ` [U-Boot] [PATCH] gadget: f_thor: Fix memory leaks of usb request and its buffer Seung-Woo Kim
2018-05-24 2:28 ` [U-Boot] [PATCH v2] " Seung-Woo Kim
2018-05-24 22:52 ` Lukasz Majewski
2018-05-25 2:14 ` Seung-Woo Kim
2018-05-25 5:21 ` [U-Boot] [PATCH v3] " Seung-Woo Kim
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox