Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1261463 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 04/23] usb: gadget: f_loopback: free requests in  loopback_disable() Robert Baldyga <r.baldyga@samsung.com> - 2015-11-03 14:00 +0100
    Re: [PATCH 04/23] usb: gadget: f_loopback: free requests in  loopback_disable() Felipe Ferreri Tonello <eu@felipetonello.com> - 2015-11-04 11:20 +0100
      Re: [PATCH 04/23] usb: gadget: f_loopback: free requests in  loopback_disable() Robert Baldyga <r.baldyga@samsung.com> - 2015-11-04 12:10 +0100

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

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.

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

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

[toc] | [next] | [standalone]

#1262174

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.

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

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

[toc] | [prev] | [next] | [standalone]

#1262213

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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web