Re: [PATCH 32/32] Staging/octeon-usb/octeon-hcd.c: Compression of lines for immediate return This patch compresses two lines in to a single line in file octeon-hcd.c

[Markus BÃhme]

Nadim,

several points stand out in your patch:

On 07/22/2016 12:17 PM, Nadim almas wrote:
> if immediate return statement is found. It also removes variable
> bytes_written as
> it is no longer needed.
> 
> It is done using script Coccinelle. And coccinelle uses following
> semantic
> patch for this compression function:
> 
> @@
> expression ret;
> identifier f;
> @@
> 
> -ret =
> +return
>      f(...);
> -return ret;

The commit message is malformed. Start a new paragraph for the longer
description of what you are doing after the short one.

> 
> Signed-off-by: Nadim Almas<nadim.902@xxxxxxxxx>
> Acked-by: Julia Lawall <julia.lawall@xxxxxxx>

If Julia acked this patch she probably should be copied on this mail.

> ---
>  Makefile                                |  2 +-
>  drivers/staging/octeon-usb/octeon-hcd.c | 16 ++++++++--------
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 4fb6bea..3d9d77a6 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1,7 +1,7 @@
>  VERSION = 4
>  PATCHLEVEL = 7
>  SUBLEVEL = 0
> -EXTRAVERSION = -rc4
> +EXTRAVERSION = -eudyptula-rc4

No need to change this.

>  NAME = Psychotic Stoned Sheep
>  
>  # *DOCUMENTATION*
> diff --git a/drivers/staging/octeon-usb/octeon-hcd.c b/drivers/staging/octeon-usb/octeon-hcd.c
> index 17442b3..b801c8a 100644
> --- a/drivers/staging/octeon-usb/octeon-hcd.c
> +++ b/drivers/staging/octeon-usb/octeon-hcd.c
> @@ -508,15 +508,15 @@ static int octeon_map_urb_for_dma(struct usb_hcd *hcd, struct urb *urb,
>  {
>  	int ret;
>  
> -	ret = octeon_alloc_temp_buffer(urb, mem_flags);
> -	if (ret)
> -		return ret;
> +     
> +	if (octeon_alloc_temp_buffer(urb, mem_flags))
> +		return octeon_alloc_temp_buffer(urb, mem_flags);

This cannot possibly be correct! You are calling a function with side
effects twice. The first call in the condition might fail while the call
in the return statement might succeed, never minding the wastefulness of
two identical calls. Besides, the original code here seems fine to me.

>  
> -	ret = usb_hcd_map_urb_for_dma(hcd, urb, mem_flags);
> -	if (ret)
> +
> +	if (usb_hcd_map_urb_for_dma(hcd, urb, mem_flags))
>  		octeon_free_temp_buffer(urb);
>  
> -	return ret;
> +	return usb_hcd_map_urb_for_dma(hcd, urb, mem_flags);
>  }

Same here.

>  
>  /**
> @@ -542,8 +542,8 @@ static void octeon_unmap_urb_for_dma(struct usb_hcd *hcd, struct urb *urb)
>   */
>  static inline u32 cvmx_usb_read_csr32(struct octeon_hcd *usb, u64 address)
>  {
> -	u32 result = cvmx_read64_uint32(address ^ 4);
> -	return result;
> +	 
> +	return cvmx_read64_uint32(address ^ 4);
>  }
>  
>  /**
> 

This change looks fine, though.

Automatically generating patches does not free you from validating each
of them manually.

Regards,
Markus