On Mon, Nov 06, 2017 at 07:17:11PM +1100, Tobin C. Harding wrote:
> On Mon, Nov 06, 2017 at 09:02:21AM +0100, gre...@linuxfoundation.org wrote:
> > On Mon, Nov 06, 2017 at 03:30:48PM +1100, Tobin C. Harding wrote:
> > > On Mon, Oct 02, 2017 at 05:49:52PM +0200, gre...@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 :(
>
> Oh? I'm surprise at this comment. I have always thought limiting scope
> of local variables was seen as a good thing. Is this a style thing that
> is on case by case basis or do you never like to declare local variables
> within blocks?
It really doesn't matter as the compiler creates the same amount of
stack space (or used to, maybe newer versions are better, haven't looked
at it in a few years).
And functions shouldn't be all _that_ big that you need to limit the
scope of a local variable :)
thanks,
greg k-h
_______________________________________________
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
