[REVIEW REQUEST] staging: unisys: review request for visorbus

[gregkh at linuxfoundation.org]

On Mon, Nov 06, 2017 at 03:30:48PM +1100, Tobin C. Harding wrote:
> On Mon, Oct 02, 2017 at 05:49:52PM +0200, gregkh at linuxfoundation.org wrote:
> > On Mon, Oct 02, 2017 at 03:41:42PM +0000, Kershner, David A wrote:
> > > Hey Greg, we think the code for visorbus is ready to be moved out
> > > of staging, can you review it to see if we have missed anything?
> > 
> > I think your html email got rejected by the list :(
> > 
> > > The files include:
> > >                 /drivers/staging/unisys/visorbus/
> > >                 /drivers/staging/unisys/include/visorchannel.h
> > >                 /drivers/staging/unisys/include/visorbus.h
> > > 
> > > The directories staging/drivers/unisys/visornic, visorhba, and visorinput
> > > are not part of the review as well as the file
> > > drivers/staging/unisys/include/iochannel.h.
> > > 
> > > We currently have 5 checkpatch.pl warnings that I know about:
> > >  - 3 CHECKs in visorchannel that are due to a MACRO that gets passed a FIELD
> > >     instead of just a variable
> > > - 2 WARNINGS dealing with char * becoming static const
> > > 
> > >  
> > > 
> > > Dan Carpenter found some extra parenthesis errors that I will address as
> > > well as look through the code to fix, but I would like to ask for the review
> > > while we are working on that.
> > 
> > Sure, I'll look at doing it in a week or so when I catch up with other
> > patches in my queue.
> > 
> > Everyone else is also welcome to do review :)
> 
> cppcheck emits a few style warnings, nothing super important but FWIW
> here is a patch
> 
> ---
>  drivers/staging/unisys/visorbus/visorchannel.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visorbus/visorchannel.c b/drivers/staging/unisys/visorbus/visorchannel.c
> index aae16073ba03..2c1beddfee29 100644
> --- a/drivers/staging/unisys/visorbus/visorchannel.c
> +++ b/drivers/staging/unisys/visorbus/visorchannel.c
> @@ -131,12 +131,13 @@ int visorchannel_write(struct visorchannel *channel, ulong offset, void *dest,
>  		       ulong nbytes)
>  {
>  	size_t chdr_size = sizeof(struct channel_header);
> -	size_t copy_size;
>  
>  	if (offset + nbytes > channel->nbytes)
>  		return -EIO;
>  
>  	if (offset < chdr_size) {
> +		size_t copy_size;
> +

Ick, no, the original code here is fine.

>  		copy_size = min(chdr_size - offset, nbytes);
>  		memcpy(((char *)(&channel->chan_hdr)) + offset,
>  		       dest, copy_size);
> @@ -260,9 +261,10 @@ int visorchannel_signalremove(struct visorchannel *channel, u32 queue,
>  			      void *msg)
>  {
>  	int rc;
> -	unsigned long flags;
>  
>  	if (channel->needs_lock) {
> +		unsigned long flags;
> +

Same here, the original code is fine.

No idea why the tool wants you to move these around, you should ignore
stuff like that :(

greg k-h