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

Groups > linux.kernel > #1449003

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

Show all headers | View raw

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@gmail.com>
> Acked-by: Julia Lawall <julia.lawall@lip6.fr>

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[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 Nadim almas <nadim.902@gmail.com> - 2016-07-22 12:20 +0200
  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 <markus.boehme@mailbox.org> - 2016-07-23 23:10 +0200
    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 Julia Lawall <julia.lawall@lip6.fr> - 2016-07-23 23:20 +0200

csiph-web