Re: [PATCH 04/23] usb: gadget: f_loopback: free requests in loopback_disable()

[Robert Baldyga <r.baldyga@samsung.com>]

On 11/04/2015 11:15 AM, Felipe Ferreri Tonello wrote:
> 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.
> 

I'm planning to convert all USB Functions to new API anyway, which
involves lots of refactoring, so it can be done en passant. Moreover
patch 14 of this series introduces clear_alt() operation, which is more
suitable place to free resources allocated in set_alt().

Thanks,
Robert Baldyga

>>
>> Signed-off-by: Robert Baldyga <r.baldyga@samsung.com>
>> ---
>>  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)
>>
>