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@xxxxxxxxx>
> > Acked-by: Julia Lawall <julia.lawall@xxxxxxx>
>
> 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
>_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
