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

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

[Kershner, David A]

[-- Attachment #1.1: Type: text/plain, Size: 937 bytes --]

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?

 

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. 

 

 

Thanks, 

David Kershner


[-- Attachment #1.2: Type: text/html, Size: 3475 bytes --]

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 7862 bytes --]

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

[gregkh]

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 :)

thanks,

greg k-h

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

[gregkh]

On Mon, Oct 02, 2017 at 05:49:52PM +0200, gregkh@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 :)

Well, don't everone jump in at once :(

Anyway, drivers/staging/unisys/visorbus/ looks good, after my one tiny
patch I've just sent you.  Feel free to send a patch moving it to
drivers/visorbus now if you want to.

Then the individual drivers can go into the different subsystem
locations after they are reviewed by the different subsystem
maintainers, but that can't happen until the core moves.

thanks,

greg k-h

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

[Dan Carpenter]

drivers/staging/unisys/visorbus/visorchipset.c
   588  static void *parser_name_get(struct parser_context *ctx)
   589  {
   590          struct visor_controlvm_parameters_header *phdr;
   591  
   592          phdr = &ctx->data;
   593          if (phdr->name_offset + phdr->name_length > ctx->param_bytes)
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
I am just reviewing quickly.  This looks like a potential integer
overflow if phdr->name_offset is UINT_MAX?

   594                  return NULL;
   595          ctx->curr = (char *)&phdr + phdr->name_offset;
   596          ctx->bytes_remaining = phdr->name_length;
   597          return parser_string_get(ctx->curr, phdr->name_length);
   598  }

drivers/staging/unisys/visorbus/visorchipset.c
  1317  static struct parser_context *parser_init_stream(u64 addr, u32 bytes,
  1318                                                   bool *retry)
  1319  {
  1320          int allocbytes;
  1321          struct parser_context *ctx;
  1322          void *mapping;
  1323  
  1324          *retry = false;
  1325          /* alloc an extra byte to ensure payload is \0 terminated */
  1326          allocbytes = bytes + 1 + (sizeof(struct parser_context) -
  1327                       sizeof(struct visor_controlvm_parameters_header));
  1328          if ((chipset_dev->controlvm_payload_bytes_buffered + bytes) >

Same question.  Can this or the allocbytes calculation have an integer
overflow?

  1329               MAX_CONTROLVM_PAYLOAD_BYTES) {
  1330                  *retry = true;
  1331                  return NULL;
  1332          }
  1333          ctx = kzalloc(allocbytes, GFP_KERNEL);
  1334          if (!ctx) {
  1335                  *retry = true;
  1336                  return NULL;
  1337          }
  1338          ctx->allocbytes = allocbytes;
  1339          ctx->param_bytes = bytes;

Otherwise the code looks pretty clean to me.

regards,
dan carpenter

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

[Tobin C. Harding]

On Mon, Oct 02, 2017 at 05:49:52PM +0200, gregkh@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;
+
 		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;
+
 		spin_lock_irqsave(&channel->remove_lock, flags);
 		rc = signalremove_inner(channel, queue, msg);
 		spin_unlock_irqrestore(&channel->remove_lock, flags);
@@ -428,9 +430,10 @@ int visorchannel_signalinsert(struct visorchannel *channel, u32 queue,
 			      void *msg)
 {
 	int rc;
-	unsigned long flags;
 
 	if (channel->needs_lock) {
+		unsigned long flags;
+
 		spin_lock_irqsave(&channel->insert_lock, flags);
 		rc = signalinsert_inner(channel, queue, msg);
 		spin_unlock_irqrestore(&channel->insert_lock, flags);
-- 
2.7.4

thanks,
Tobin.

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

[gregkh]

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@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

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

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 09:02:21AM +0100, gregkh@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, gregkh@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?

thanks,
Tobin.

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

[gregkh]

On Mon, Nov 06, 2017 at 07:17:11PM +1100, Tobin C. Harding wrote:
> On Mon, Nov 06, 2017 at 09:02:21AM +0100, gregkh@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, gregkh@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

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

[Dan Carpenter]

On Mon, Nov 06, 2017 at 07:17:11PM +1100, Tobin C. Harding wrote:
> > > @@ -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?
> 

Greg has answered for himself but here are my thoughts...

If you look at Colin King's patches, he's using CPPcheck and he's pretty
religiously moving variables to the localest scope and no one complains.
It makes sense when he does it from what I have seen.  But mostly he's
definitely cleaning up the code and fixing bugs and no one cares too
much about the scope one way or the other.

But here, I agreed with Greg that the original code was better.  The
chdr_size and copy_size variables are closely related and are better
declared together.  The flags declaration was also slightly cleaner at
the start instead of mixing it up with the code.  Sometimes in a long
function it definitely makes sense to use a local declaration.  So it's
a case by case thing.

But mostly no one cares, and I don't want to see hundreds of patches
mechanically moving declarations around.

regards,
dan carpenter

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

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

[Tobin C. Harding]

On Mon, Nov 06, 2017 at 04:56:40PM +0300, Dan Carpenter wrote:
> On Mon, Nov 06, 2017 at 07:17:11PM +1100, Tobin C. Harding wrote:
> > > > @@ -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?
> > 
> 
> Greg has answered for himself but here are my thoughts...

thanks for taking the time.

> If you look at Colin King's patches, he's using CPPcheck and he's pretty
> religiously moving variables to the localest scope and no one complains.
> It makes sense when he does it from what I have seen.  But mostly he's
> definitely cleaning up the code and fixing bugs and no one cares too
> much about the scope one way or the other.
> 
> But here, I agreed with Greg that the original code was better.  The
> chdr_size and copy_size variables are closely related and are better
> declared together.  The flags declaration was also slightly cleaner at
> the start instead of mixing it up with the code.  Sometimes in a long
> function it definitely makes sense to use a local declaration.  So it's
> a case by case thing.

Cool, got it.

> But mostly no one cares, and I don't want to see hundreds of patches
> mechanically moving declarations around.

Oh bother, scrap that 400 patch series I queued up ... just kidding. 

thanks,
Tobin.
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel