[PATCH] staging: vt6656: Use common error handling code in vnt_alloc_bufs()

[PATCH] staging: vt6656: Use common error handling code in vnt_alloc_bufs()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 23 Aug 2019 15:15:41 +0200

Adjust jump targets so that a bit of exception handling can be better
reused at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/vt6656/main_usb.c | 46 +++++++++++++------------------
 1 file changed, 19 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
index 856ba97aec4f..d9f14da37bbc 100644
--- a/drivers/staging/vt6656/main_usb.c
+++ b/drivers/staging/vt6656/main_usb.c
@@ -443,10 +443,8 @@ static int vnt_alloc_bufs(struct vnt_private *priv)

 	for (ii = 0; ii < priv->num_tx_context; ii++) {
 		tx_context = kmalloc(sizeof(*tx_context), GFP_KERNEL);
-		if (!tx_context) {
-			ret = -ENOMEM;
-			goto free_tx;
-		}
+		if (!tx_context)
+			goto e_nomem_tx;

 		priv->tx_context[ii] = tx_context;
 		tx_context->priv = priv;
@@ -454,20 +452,16 @@ static int vnt_alloc_bufs(struct vnt_private *priv)

 		/* allocate URBs */
 		tx_context->urb = usb_alloc_urb(0, GFP_KERNEL);
-		if (!tx_context->urb) {
-			ret = -ENOMEM;
-			goto free_tx;
-		}
+		if (!tx_context->urb)
+			goto e_nomem_tx;

 		tx_context->in_use = false;
 	}

 	for (ii = 0; ii < priv->num_rcb; ii++) {
 		priv->rcb[ii] = kzalloc(sizeof(*priv->rcb[ii]), GFP_KERNEL);
-		if (!priv->rcb[ii]) {
-			ret = -ENOMEM;
-			goto free_rx_tx;
-		}
+		if (!priv->rcb[ii])
+			goto e_nomem_rx;

 		rcb = priv->rcb[ii];

@@ -475,16 +469,12 @@ static int vnt_alloc_bufs(struct vnt_private *priv)

 		/* allocate URBs */
 		rcb->urb = usb_alloc_urb(0, GFP_KERNEL);
-		if (!rcb->urb) {
-			ret = -ENOMEM;
-			goto free_rx_tx;
-		}
+		if (!rcb->urb)
+			goto e_nomem_rx;

 		rcb->skb = dev_alloc_skb(priv->rx_buf_sz);
-		if (!rcb->skb) {
-			ret = -ENOMEM;
-			goto free_rx_tx;
-		}
+		if (!rcb->skb)
+			goto e_nomem_rx;

 		rcb->in_use = false;

@@ -495,21 +485,23 @@ static int vnt_alloc_bufs(struct vnt_private *priv)
 	}

 	priv->interrupt_urb = usb_alloc_urb(0, GFP_KERNEL);
-	if (!priv->interrupt_urb) {
-		ret = -ENOMEM;
-		goto free_rx_tx;
-	}
+	if (!priv->interrupt_urb)
+		goto e_nomem_rx;

 	priv->int_buf.data_buf = kmalloc(MAX_INTERRUPT_SIZE, GFP_KERNEL);
-	if (!priv->int_buf.data_buf) {
-		ret = -ENOMEM;
+	if (!priv->int_buf.data_buf)
 		goto free_rx_tx_urb;
-	}

 	return 0;

+e_nomem_tx:
+	ret = -ENOMEM;
+	goto free_tx;
+
 free_rx_tx_urb:
 	usb_free_urb(priv->interrupt_urb);
+e_nomem_rx:
+	ret = -ENOMEM;
 free_rx_tx:
 	vnt_free_rx_bufs(priv);
 free_tx:
--
2.23.0

[PATCH] staging: vt6656: Use common error handling code in vnt_alloc_bufs()

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Fri, 23 Aug 2019 15:15:41 +0200

Adjust jump targets so that a bit of exception handling can be better
reused at the end of this function.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/staging/vt6656/main_usb.c | 46 +++++++++++++------------------
 1 file changed, 19 insertions(+), 27 deletions(-)

diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
index 856ba97aec4f..d9f14da37bbc 100644
--- a/drivers/staging/vt6656/main_usb.c
+++ b/drivers/staging/vt6656/main_usb.c
@@ -443,10 +443,8 @@ static int vnt_alloc_bufs(struct vnt_private *priv)

 	for (ii = 0; ii < priv->num_tx_context; ii++) {
 		tx_context = kmalloc(sizeof(*tx_context), GFP_KERNEL);
-		if (!tx_context) {
-			ret = -ENOMEM;
-			goto free_tx;
-		}
+		if (!tx_context)
+			goto e_nomem_tx;

 		priv->tx_context[ii] = tx_context;
 		tx_context->priv = priv;
@@ -454,20 +452,16 @@ static int vnt_alloc_bufs(struct vnt_private *priv)

 		/* allocate URBs */
 		tx_context->urb = usb_alloc_urb(0, GFP_KERNEL);
-		if (!tx_context->urb) {
-			ret = -ENOMEM;
-			goto free_tx;
-		}
+		if (!tx_context->urb)
+			goto e_nomem_tx;

 		tx_context->in_use = false;
 	}

 	for (ii = 0; ii < priv->num_rcb; ii++) {
 		priv->rcb[ii] = kzalloc(sizeof(*priv->rcb[ii]), GFP_KERNEL);
-		if (!priv->rcb[ii]) {
-			ret = -ENOMEM;
-			goto free_rx_tx;
-		}
+		if (!priv->rcb[ii])
+			goto e_nomem_rx;

 		rcb = priv->rcb[ii];

@@ -475,16 +469,12 @@ static int vnt_alloc_bufs(struct vnt_private *priv)

 		/* allocate URBs */
 		rcb->urb = usb_alloc_urb(0, GFP_KERNEL);
-		if (!rcb->urb) {
-			ret = -ENOMEM;
-			goto free_rx_tx;
-		}
+		if (!rcb->urb)
+			goto e_nomem_rx;

 		rcb->skb = dev_alloc_skb(priv->rx_buf_sz);
-		if (!rcb->skb) {
-			ret = -ENOMEM;
-			goto free_rx_tx;
-		}
+		if (!rcb->skb)
+			goto e_nomem_rx;

 		rcb->in_use = false;

@@ -495,21 +485,23 @@ static int vnt_alloc_bufs(struct vnt_private *priv)
 	}

 	priv->interrupt_urb = usb_alloc_urb(0, GFP_KERNEL);
-	if (!priv->interrupt_urb) {
-		ret = -ENOMEM;
-		goto free_rx_tx;
-	}
+	if (!priv->interrupt_urb)
+		goto e_nomem_rx;

 	priv->int_buf.data_buf = kmalloc(MAX_INTERRUPT_SIZE, GFP_KERNEL);
-	if (!priv->int_buf.data_buf) {
-		ret = -ENOMEM;
+	if (!priv->int_buf.data_buf)
 		goto free_rx_tx_urb;
-	}

 	return 0;

+e_nomem_tx:
+	ret = -ENOMEM;
+	goto free_tx;
+
 free_rx_tx_urb:
 	usb_free_urb(priv->interrupt_urb);
+e_nomem_rx:
+	ret = -ENOMEM;
 free_rx_tx:
 	vnt_free_rx_bufs(priv);
 free_tx:
--
2.23.0

Re: [PATCH] staging: vt6656: Use common error handling code in vnt_alloc_bufs()

[Quentin Deslandes]

On Fri, Aug 23, 2019 at 03:30:11PM +0200, Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Fri, 23 Aug 2019 15:15:41 +0200
> 
> Adjust jump targets so that a bit of exception handling can be better
> reused at the end of this function.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/staging/vt6656/main_usb.c | 46 +++++++++++++------------------
>  1 file changed, 19 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/staging/vt6656/main_usb.c b/drivers/staging/vt6656/main_usb.c
> index 856ba97aec4f..d9f14da37bbc 100644
> --- a/drivers/staging/vt6656/main_usb.c
> +++ b/drivers/staging/vt6656/main_usb.c
> @@ -443,10 +443,8 @@ static int vnt_alloc_bufs(struct vnt_private *priv)
> 
>  	for (ii = 0; ii < priv->num_tx_context; ii++) {
>  		tx_context = kmalloc(sizeof(*tx_context), GFP_KERNEL);
> -		if (!tx_context) {
> -			ret = -ENOMEM;
> -			goto free_tx;
> -		}
> +		if (!tx_context)
> +			goto e_nomem_tx;
> 
>  		priv->tx_context[ii] = tx_context;
>  		tx_context->priv = priv;
> @@ -454,20 +452,16 @@ static int vnt_alloc_bufs(struct vnt_private *priv)
> 
>  		/* allocate URBs */
>  		tx_context->urb = usb_alloc_urb(0, GFP_KERNEL);
> -		if (!tx_context->urb) {
> -			ret = -ENOMEM;
> -			goto free_tx;
> -		}
> +		if (!tx_context->urb)
> +			goto e_nomem_tx;
> 
>  		tx_context->in_use = false;
>  	}
> 
>  	for (ii = 0; ii < priv->num_rcb; ii++) {
>  		priv->rcb[ii] = kzalloc(sizeof(*priv->rcb[ii]), GFP_KERNEL);
> -		if (!priv->rcb[ii]) {
> -			ret = -ENOMEM;
> -			goto free_rx_tx;
> -		}
> +		if (!priv->rcb[ii])
> +			goto e_nomem_rx;
> 
>  		rcb = priv->rcb[ii];
> 
> @@ -475,16 +469,12 @@ static int vnt_alloc_bufs(struct vnt_private *priv)
> 
>  		/* allocate URBs */
>  		rcb->urb = usb_alloc_urb(0, GFP_KERNEL);
> -		if (!rcb->urb) {
> -			ret = -ENOMEM;
> -			goto free_rx_tx;
> -		}
> +		if (!rcb->urb)
> +			goto e_nomem_rx;
> 
>  		rcb->skb = dev_alloc_skb(priv->rx_buf_sz);
> -		if (!rcb->skb) {
> -			ret = -ENOMEM;
> -			goto free_rx_tx;
> -		}
> +		if (!rcb->skb)
> +			goto e_nomem_rx;
> 
>  		rcb->in_use = false;
> 
> @@ -495,21 +485,23 @@ static int vnt_alloc_bufs(struct vnt_private *priv)
>  	}
> 
>  	priv->interrupt_urb = usb_alloc_urb(0, GFP_KERNEL);
> -	if (!priv->interrupt_urb) {
> -		ret = -ENOMEM;
> -		goto free_rx_tx;
> -	}
> +	if (!priv->interrupt_urb)
> +		goto e_nomem_rx;
> 
>  	priv->int_buf.data_buf = kmalloc(MAX_INTERRUPT_SIZE, GFP_KERNEL);
> -	if (!priv->int_buf.data_buf) {
> -		ret = -ENOMEM;
> +	if (!priv->int_buf.data_buf)
>  		goto free_rx_tx_urb;
> -	}
> 
>  	return 0;
> 
> +e_nomem_tx:
> +	ret = -ENOMEM;
> +	goto free_tx;
> +
>  free_rx_tx_urb:
>  	usb_free_urb(priv->interrupt_urb);
> +e_nomem_rx:
> +	ret = -ENOMEM;
>  free_rx_tx:
>  	vnt_free_rx_bufs(priv);
>  free_tx:
> --
> 2.23.0
> 

Your patch remove redundant code, which is fine. However, and IMHO, the
code you changed was simple enough to be understand quickly. I think replacing
it with a crossed goto (even if it remove redundant code) might not be the best
option.

A solution might be to move the second loop to the top of the function so
you should be able to replace the end of the cleanup calls with:

enomem:
	ret = -ENOMEM;
free_rx:
	vnt_free_rx_bufs(priv);
	return ret;

This way, only a failed call to vnt_submit_rx_urb() should jump to
free_rx, another failed call should jump to enomem or previously defined
label, so we can correctly forward errors. With such solution it might
be worth adding a comment to describe that all error should be ENOMEM
except for vnt_submit_rx_urb(). Does that looks good to you?

Regards,
Quentin

Re: staging: vt6656: Use common error handling code in vnt_alloc_bufs()

[Markus Elfring]

 Your patch remove redundant code, which is fine.

Thanks for your constructive feedback.


> code you changed was simple enough to be understand quickly. I think replacing
> it with a crossed goto (even if it remove redundant code) might not be the best
> option.
>
> A solution might be to move the second loop to the top of the function

I am unsure about the relevance of the loop ordering for the affected
resource allocations.


> so you should be able to replace the end of the cleanup calls with:
>
> enomem:
> 	ret = -ENOMEM;
> free_rx:
> 	vnt_free_rx_bufs(priv);
> 	return ret;

The exception handling can eventually adjusted another bit according
to your refactoring.


> This way, only a failed call to vnt_submit_rx_urb() should jump to free_rx,

It seems that a goto statement will still be needed in an error code
part by both discussed variants.


> another failed call should jump to enomem or previously defined
> label, so we can correctly forward errors.

This view sounds promising.


> With such solution it might be worth adding a comment to describe
> that all error should be ENOMEM except for vnt_submit_rx_urb().

Can this function implementation become clearer also without
such a comment?

Regards,
Markus

Re: staging: vt6656: Use common error handling code in vnt_alloc_bufs()

[Markus Elfring]

 Your patch remove redundant code, which is fine.

Thanks for your constructive feedback.


> code you changed was simple enough to be understand quickly. I think replacing
> it with a crossed goto (even if it remove redundant code) might not be the best
> option.
>
> A solution might be to move the second loop to the top of the function

I am unsure about the relevance of the loop ordering for the affected
resource allocations.


> so you should be able to replace the end of the cleanup calls with:
>
> enomem:
> 	ret = -ENOMEM;
> free_rx:
> 	vnt_free_rx_bufs(priv);
> 	return ret;

The exception handling can eventually adjusted another bit according
to your refactoring.


> This way, only a failed call to vnt_submit_rx_urb() should jump to free_rx,

It seems that a goto statement will still be needed in an error code
part by both discussed variants.


> another failed call should jump to enomem or previously defined
> label, so we can correctly forward errors.

This view sounds promising.


> With such solution it might be worth adding a comment to describe
> that all error should be ENOMEM except for vnt_submit_rx_urb().

Can this function implementation become clearer also without
such a comment?

Regards,
Markus

Re: [PATCH] staging: vt6656: Use common error handling code in vnt_alloc_bufs()

[Dan Carpenter]

The original code is fine.

There was no chance we were going to apply the patch so there is no need
for any discussion.  I don't know why Markus sent it when he knows.

regards,
dan carpenter

Re: [PATCH] staging: vt6656: Use common error handling code in vnt_alloc_bufs()

[Dan Carpenter]

The original code is fine.

There was no chance we were going to apply the patch so there is no need
for any discussion.  I don't know why Markus sent it when he knows.

regards,
dan carpenter