Subject: Re: [PATCH 04/23] usb: gadget: f_loopback: free requests in
 loopback_disable()
To: Felipe Ferreri Tonello <eu@felipetonello.com>, balbi@ti.com
References: <1446555242-3733-1-git-send-email-r.baldyga@samsung.com>
 <1446555242-3733-5-git-send-email-r.baldyga@samsung.com>
 <5639DAD8.40204@felipetonello.com>
Cc: gregkh@linuxfoundation.org, andrzej.p@samsung.com,
        m.szyprowski@samsung.com, b.zolnierkie@samsung.com,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
From: Robert Baldyga <r.baldyga@samsung.com>
Message-id: <5639E5C7.8000201@samsung.com>
Date: Wed, 04 Nov 2015 12:02:31 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101
 Thunderbird/38.3.0
MIME-version: 1.0
In-reply-to: <5639DAD8.40204@felipetonello.com>
Content-type: text/plain; charset=windows-1252
Content-transfer-encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5972
Lines: 173

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/