Messages in this thread | | | Date | Sat, 23 Jul 2016 23:10:42 +0200 (CEST) | From | Julia Lawall <> | Subject | 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 |
| |
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 >
| |