[U-Boot] [PATCH v2] gadget: f_thor: Fix memory leaks of usb request and its buffer

[Seung-Woo Kim <sw0312.kim@samsung.com>]

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
--