[PATCH 1/2] staging: gdm724x: Remove initialisation

[PATCH 1/2] staging: gdm724x: Remove initialisation

[Nishka Dasgupta]

The initial value of return variable ret, -1, is never used and hence
can be removed.
Issue found with Coccinelle.

Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com>
---
 drivers/staging/gdm724x/gdm_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/gdm724x/gdm_usb.c b/drivers/staging/gdm724x/gdm_usb.c
index dc4da66c3695..d023f83f9097 100644
--- a/drivers/staging/gdm724x/gdm_usb.c
+++ b/drivers/staging/gdm724x/gdm_usb.c
@@ -60,7 +60,7 @@ static int request_mac_address(struct lte_udev *udev)
 	struct hci_packet *hci = (struct hci_packet *)buf;
 	struct usb_device *usbdev = udev->usbdev;
 	int actual;
-	int ret = -1;
+	int ret;
 
 	hci->cmd_evt = gdm_cpu_to_dev16(udev->gdm_ed, LTE_GET_INFORMATION);
 	hci->len = gdm_cpu_to_dev16(udev->gdm_ed, 1);
-- 
2.19.1

[PATCH 2/2] staging: gdm724x: Remove variable

[Nishka Dasgupta]

The return variable is used only twice (in two different branches), and
both times it is assigned the same constant value. These can therefore
be merged into the same assignment, placed at the point that both
these branches (and no other) go to. The return variable itself can be
removed.
Issue found with Coccinelle.

Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com>
---
 drivers/staging/gdm724x/gdm_usb.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/staging/gdm724x/gdm_usb.c b/drivers/staging/gdm724x/gdm_usb.c
index d023f83f9097..8145ae2afba7 100644
--- a/drivers/staging/gdm724x/gdm_usb.c
+++ b/drivers/staging/gdm724x/gdm_usb.c
@@ -295,7 +295,6 @@ static void release_usb(struct lte_udev *udev)
 
 static int init_usb(struct lte_udev *udev)
 {
-	int ret = 0;
 	int i;
 	struct tx_cxt *tx = &udev->tx;
 	struct rx_cxt *rx = &udev->rx;
@@ -326,7 +325,6 @@ static int init_usb(struct lte_udev *udev)
 	for (i = 0; i < MAX_NUM_SDU_BUF; i++) {
 		t_sdu = alloc_tx_sdu_struct();
 		if (!t_sdu) {
-			ret = -ENOMEM;
 			goto fail;
 		}
 
@@ -337,7 +335,6 @@ static int init_usb(struct lte_udev *udev)
 	for (i = 0; i < MAX_RX_SUBMIT_COUNT * 2; i++) {
 		r = alloc_rx_struct();
 		if (!r) {
-			ret = -ENOMEM;
 			goto fail;
 		}
 
@@ -349,7 +346,7 @@ static int init_usb(struct lte_udev *udev)
 	return 0;
 fail:
 	release_usb(udev);
-	return ret;
+	return -ENOMEM;
 }
 
 static int set_mac_address(u8 *data, void *arg)
-- 
2.19.1

Re: [PATCH 2/2] staging: gdm724x: Remove variable

[Greg KH]

On Fri, May 24, 2019 at 11:30:26AM +0530, Nishka Dasgupta wrote:
> The return variable is used only twice (in two different branches), and
> both times it is assigned the same constant value. These can therefore
> be merged into the same assignment, placed at the point that both
> these branches (and no other) go to. The return variable itself can be
> removed.
> Issue found with Coccinelle.
> 
> Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com>
> ---
>  drivers/staging/gdm724x/gdm_usb.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)

Your subject line needs a lot of work.

How about:
	staging: gdm724x: remove unneeded variable in init_usb()



> 
> diff --git a/drivers/staging/gdm724x/gdm_usb.c b/drivers/staging/gdm724x/gdm_usb.c
> index d023f83f9097..8145ae2afba7 100644
> --- a/drivers/staging/gdm724x/gdm_usb.c
> +++ b/drivers/staging/gdm724x/gdm_usb.c
> @@ -295,7 +295,6 @@ static void release_usb(struct lte_udev *udev)
>  
>  static int init_usb(struct lte_udev *udev)
>  {
> -	int ret = 0;
>  	int i;
>  	struct tx_cxt *tx = &udev->tx;
>  	struct rx_cxt *rx = &udev->rx;
> @@ -326,7 +325,6 @@ static int init_usb(struct lte_udev *udev)
>  	for (i = 0; i < MAX_NUM_SDU_BUF; i++) {
>  		t_sdu = alloc_tx_sdu_struct();
>  		if (!t_sdu) {
> -			ret = -ENOMEM;
>  			goto fail;
>  		}
>  
> @@ -337,7 +335,6 @@ static int init_usb(struct lte_udev *udev)
>  	for (i = 0; i < MAX_RX_SUBMIT_COUNT * 2; i++) {
>  		r = alloc_rx_struct();
>  		if (!r) {
> -			ret = -ENOMEM;
>  			goto fail;
>  		}
>  
> @@ -349,7 +346,7 @@ static int init_usb(struct lte_udev *udev)
>  	return 0;
>  fail:
>  	release_usb(udev);
> -	return ret;
> +	return -ENOMEM;
>  }

Again, you are getting _really_ close to the "churn for churn sake".
Maybe this is needed, but it's really a fine line...

thanks,

greg k-h

Re: [PATCH 1/2] staging: gdm724x: Remove initialisation

[Greg KH]

On Fri, May 24, 2019 at 11:30:25AM +0530, Nishka Dasgupta wrote:
> The initial value of return variable ret, -1, is never used and hence
> can be removed.
> Issue found with Coccinelle.
> 
> Signed-off-by: Nishka Dasgupta <nishkadg.linux@gmail.com>
> ---
>  drivers/staging/gdm724x/gdm_usb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Again, fix up the subject line.

thanks,

greg k-h

Re: [PATCH 2/2] staging: gdm724x: Remove variable

[Sven Van Asbroeck]

On Fri, May 24, 2019 at 2:04 AM Nishka Dasgupta
<nishkadg.linux@gmail.com> wrote:
>
> The return variable is used only twice (in two different branches), and
> both times it is assigned the same constant value. These can therefore
> be merged into the same assignment, placed at the point that both
> these branches (and no other) go to. The return variable itself can be
> removed.

>  fail:
>         release_usb(udev);
> -       return ret;
> +       return -ENOMEM;
>  }

At the risk of sticking my nose where it doesn't belong...

AFAIK it's a well-established pattern to have a success path returning 0,
and an error path returning ret, where ret gets assigned the err value.

This patch removes the pattern, making it slightly harder for developers
to read. And if the function needs to return different err values in the
future, that future patch will need to add the ret variable back in.

Modern compilers optimize ret away, so the patch won't result in
smaller or more efficient code.

This particular patch sounds like negative work to me.