gadget: f_thor: Fix memory leaks of usb request and its buffer
A message that is not in the archive (sent off-list or not yet received): CGME20180523072552epcas1p3b1bdbc7e0f49053fe3ce15117e61b429@epcas1p3.samsung.com
PATCH 1/1 gadget: f_thor: Fix memory leaks of usb request and its buffer
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.
--- drivers/usb/gadget/f_thor.c | 34 +++++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 9 deletions(-)
drivers/usb/gadget/f_thor.c +25 -9
@@ -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;
Signature · 1 line
1.7.9.5
PATCH v2 1/1 gadget: f_thor: Fix memory leaks of usb request and its buffer
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.
---
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(-)
drivers/usb/gadget/f_thor.c +29 -16
@@ -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; }
Signature · 1 line
1.7.9.5
PATCH v2 1/1 gadget: f_thor: Fix memory leaks of usb request and its buffer
Hi Seung-Woo,
Quoted 44 lines
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 <[email protected]>---
Change from v1
- remove allocation ofout_eprequest 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(-)
drivers/usb/gadget/f_thor.c +9
-2@@ -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) -{
drivers/usb/gadget/f_thor.c static int thor_func_bind(struct · line 881
Quoted 10 lines
drivers/usb/gadget/f_thor.c +1
-4-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.
drivers/usb/gadget/f_thor.c static void thor_func_disable(struct usb_function · line 899
Quoted 9 lines
drivers/usb/gadget/f_thor.c +0
-2free (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).
Quoted 21 lines
drivers/usb/gadget/f_thor.c +2
-3usb_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;
drivers/usb/gadget/f_thor.c static int thor_eps_setup(struct usb_function · line 948
Quoted 9 lines
drivers/usb/gadget/f_thor.c +2
-2@@ -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?
Quoted 20 lines
drivers/usb/gadget/f_thor.c +8
-3if (!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:
drivers/usb/gadget/f_thor.c static int thor_eps_setup(struct usb_function *f) · line 975
Quoted 10 lines
drivers/usb/gadget/f_thor.c +7
-0+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>
PATCH v2 1/1 gadget: f_thor: Fix memory leaks of usb request and its buffer
Hello Lukasz,
Quoted 65 lines · Lukasz Majewski
Hi Seung-Woo,
Quoted 44 lines
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 <[email protected]>---
Change from v1
- remove allocation ofout_eprequest 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(-)
drivers/usb/gadget/f_thor.c +9
-2@@ -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) -{drivers/usb/gadget/f_thor.c static int thor_func_bind(struct · line 881
Quoted 10 lines
drivers/usb/gadget/f_thor.c +1
-4-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.
Quoted 6 lines
drivers/usb/gadget/f_thor.c +0
-0free (dev);memset (thor_func,0 ,sizeof (*thor_func)); thor_func =NULL ;@@ -895,8 +899,6 @@ static void thor_func_disable(struct usb_function *f) }
Quoted 7 lines
drivers/usb/gadget/f_thor.c static void thor_func_disable(struct usb_function *f) · line 899
Quoted 3 lines
drivers/usb/gadget/f_thor.c +0
-2if (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.
Quoted 28 lines
Quoted 28 lines
drivers/usb/gadget/f_thor.c +3
-4usb_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;
Quoted 6 lines
drivers/usb/gadget/f_thor.c static int thor_eps_setup(struct usb_function *f) · line 948
Quoted 3 lines
drivers/usb/gadget/f_thor.c +1
-1ep->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.
Quoted 5 lines
( I do see the
req->complete=thor_rx_tx_completebelow ).If the
thor_start_epcan 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
Quoted 45 lines
Quoted 21 lines
drivers/usb/gadget/f_thor.c +8
-3if (!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:drivers/usb/gadget/f_thor.c static int thor_eps_setup(struct usb_function *f) · line 975
Quoted 10 lines
drivers/usb/gadget/f_thor.c +7
-0+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
Signature · 3 lines
Seung-Woo Kim Samsung Research --
PATCH v3 1/1 gadget: f_thor: Fix memory leaks of usb request and its buffer
In reply to Seung-Woo KimThere 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.
---
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(-)
drivers/usb/gadget/f_thor.c +32 -33
@@ -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; }
Signature · 1 line
1.7.9.5