* [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
@ 2016-07-22 10:17 Nadim almas
2016-07-23 21:02 ` Markus Böhme
0 siblings, 1 reply; 3+ messages in thread
From: Nadim almas @ 2016-07-22 10:17 UTC (permalink / raw)
To: gregkh; +Cc: mmarek, linux-kernel, devel, Nadim almas
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;
Signed-off-by: Nadim Almas<nadim.902@gmail.com>
Acked-by: Julia Lawall <julia.lawall@lip6.fr>
---
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
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);
- 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);
}
/**
@@ -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);
}
/**
--
2.7.4
^ permalink raw reply [flat|nested] 3+ messages in thread
* 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
2016-07-22 10:17 [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
@ 2016-07-23 21:02 ` Markus Böhme
2016-07-23 21:10 ` Julia Lawall
0 siblings, 1 reply; 3+ messages in thread
From: Markus Böhme @ 2016-07-23 21:02 UTC (permalink / raw)
To: Nadim almas; +Cc: gregkh, devel, mmarek, linux-kernel, julia.lawall
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* 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
2016-07-23 21:02 ` Markus Böhme
@ 2016-07-23 21:10 ` Julia Lawall
0 siblings, 0 replies; 3+ messages in thread
From: Julia Lawall @ 2016-07-23 21:10 UTC (permalink / raw)
To: Markus Böhme; +Cc: Nadim almas, gregkh, devel, mmarek, linux-kernel
[-- Attachment #1: Type: TEXT/PLAIN, Size: 3106 bytes --]
On Sat, 23 Jul 2016, Markus Böhme wrote:
> 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.
No, I haven't seen it... And I don't think the above semantic patch
should have made these changes.
julia
>
> > ---
> > 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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-07-23 21:10 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-07-22 10:17 [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
2016-07-23 21:02 ` Markus Böhme
2016-07-23 21:10 ` Julia Lawall
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®