Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1658841 > unrolled thread

Re: [PATCH 0/3] move visorbus out of staging to drivers/virt/visorbus

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 0/3] move visorbus out of staging to drivers/virt/visorbus Greg KH <gregkh@linuxfoundation.org> - 2017-06-06 16:50 +0200
    Re: [PATCH 0/3] move visorbus out of staging to drivers/virt/visorbus Greg KH <gregkh@linuxfoundation.org> - 2017-06-06 17:00 +0200
      Re: [PATCH 0/3] move visorbus out of staging to drivers/virt/visorbus Greg KH <gregkh@linuxfoundation.org> - 2017-06-06 17:10 +0200
        RE: [PATCH 0/3] move visorbus out of staging to drivers/virt/visorbus "Kershner, David A" <David.Kershner@unisys.com> - 2017-06-06 20:30 +0200
          Re: [PATCH 0/3] move visorbus out of staging to drivers/virt/visorbus Greg KH <gregkh@linuxfoundation.org> - 2017-06-06 21:00 +0200
    Re: [PATCH 0/3] move visorbus out of staging to drivers/virt/visorbus Greg KH <gregkh@linuxfoundation.org> - 2017-06-06 17:00 +0200
      Re: [PATCH 0/3] move visorbus out of staging to  drivers/virt/visorbus Joe Perches <joe@perches.com> - 2017-06-06 17:40 +0200
        Re: [PATCH 0/3] move visorbus out of staging to drivers/virt/visorbus Greg KH <gregkh@linuxfoundation.org> - 2017-06-06 17:40 +0200
          Re: [PATCH 0/3] move visorbus out of staging to  drivers/virt/visorbus Joe Perches <joe@perches.com> - 2017-06-06 18:00 +0200
            Re: [PATCH 0/3] move visorbus out of staging to drivers/virt/visorbus Greg KH <gregkh@linuxfoundation.org> - 2017-06-06 18:10 +0200
    Re: [PATCH 0/3] move visorbus out of staging to drivers/virt/visorbus Christoph Hellwig <hch@infradead.org> - 2017-06-07 16:10 +0200

#1658841 — Re: [PATCH 0/3] move visorbus out of staging to drivers/virt/visorbus

On Mon, Jun 05, 2017 at 04:07:29PM -0400, David Kershner wrote:
> This patchset moves drivers/staging/unisys/include to
> include/linux/visorbus, and moves drivers/staging/unisys/visorbus to
> drivers/virt/visorbus.

Um, are you thinking it is ready to be moved?  Have you asked for
another review?

In a totally random chance, I was doing some driver core work today and
I noticed that in drivers/staging/unisys/visorbus/visorbus_main.c, you
have 2 tabs for your 'struct attribute' variables, which is really odd.

Also, you should be using the ATTRIBUTE_GROUPS() macro for them instead
of having to "open code" the struct attribute_group lists.

So either you all have horrible luck in that I just happened to find the
only remaining problem, or that you should proabably ask for a good code
audit, I haven't looked at the code before today since the last round of
"fun" I found in just one other random file :)

thanks,

greg k-h

[toc] | [next] | [standalone]

#1658843

On Tue, Jun 06, 2017 at 04:53:22PM +0200, Greg KH wrote:
> On Tue, Jun 06, 2017 at 04:49:09PM +0200, Greg KH wrote:
> > On Mon, Jun 05, 2017 at 04:07:29PM -0400, David Kershner wrote:
> > > This patchset moves drivers/staging/unisys/include to
> > > include/linux/visorbus, and moves drivers/staging/unisys/visorbus to
> > > drivers/virt/visorbus.
> > 
> > Um, are you thinking it is ready to be moved?  Have you asked for
> > another review?
> > 
> > In a totally random chance, I was doing some driver core work today and
> > I noticed that in drivers/staging/unisys/visorbus/visorbus_main.c, you
> > have 2 tabs for your 'struct attribute' variables, which is really odd.
> > 
> > Also, you should be using the ATTRIBUTE_GROUPS() macro for them instead
> > of having to "open code" the struct attribute_group lists.
> > 
> > So either you all have horrible luck in that I just happened to find the
> > only remaining problem, or that you should proabably ask for a good code
> > audit, I haven't looked at the code before today since the last round of
> > "fun" I found in just one other random file :)
> 
> Also, many of the attribute callbacks in that file seem to all have
> their leading '{' in the wrong place.  Odd that checkpatch.pl doesn't
> catch that...
> 
> partition_handle_show() is one such example that is obviously wrong.
> 
> There's also one checkpatch.pl warning for it, which should probably be
> resolved as well.

drivers/staging/unisys/visorbus/visorbus_main.c:1035: WARNING: Prefer using '"%s...", __func__' to using 'create_bus_instance', this function's name, in a string

to be specific, something you should have caught, right?

Are you sure this is ready to be moved out of staging?  :(

greg k-h

[toc] | [prev] | [next] | [standalone]

#1658854

On Tue, Jun 06, 2017 at 04:54:30PM +0200, Greg KH wrote:
> On Tue, Jun 06, 2017 at 04:53:22PM +0200, Greg KH wrote:
> > On Tue, Jun 06, 2017 at 04:49:09PM +0200, Greg KH wrote:
> > > On Mon, Jun 05, 2017 at 04:07:29PM -0400, David Kershner wrote:
> > > > This patchset moves drivers/staging/unisys/include to
> > > > include/linux/visorbus, and moves drivers/staging/unisys/visorbus to
> > > > drivers/virt/visorbus.
> > > 
> > > Um, are you thinking it is ready to be moved?  Have you asked for
> > > another review?
> > > 
> > > In a totally random chance, I was doing some driver core work today and
> > > I noticed that in drivers/staging/unisys/visorbus/visorbus_main.c, you
> > > have 2 tabs for your 'struct attribute' variables, which is really odd.
> > > 
> > > Also, you should be using the ATTRIBUTE_GROUPS() macro for them instead
> > > of having to "open code" the struct attribute_group lists.
> > > 
> > > So either you all have horrible luck in that I just happened to find the
> > > only remaining problem, or that you should proabably ask for a good code
> > > audit, I haven't looked at the code before today since the last round of
> > > "fun" I found in just one other random file :)
> > 
> > Also, many of the attribute callbacks in that file seem to all have
> > their leading '{' in the wrong place.  Odd that checkpatch.pl doesn't
> > catch that...
> > 
> > partition_handle_show() is one such example that is obviously wrong.
> > 
> > There's also one checkpatch.pl warning for it, which should probably be
> > resolved as well.
> 
> drivers/staging/unisys/visorbus/visorbus_main.c:1035: WARNING: Prefer using '"%s...", __func__' to using 'create_bus_instance', this function's name, in a string
> 
> to be specific, something you should have caught, right?
> 
> Are you sure this is ready to be moved out of staging?  :(

Eek, I can't look away...

You do this a bunch:
	if (dev->visorchannel) {
		visorchannel_destroy(dev->visorchannel);

yet the first thing that visorchannel_destroy() does is check for null.
So, no need to test this twice, right, only do so in the function, that
will make your code flow a lot "smoother" where ever you are calling
this.

Ok, I'll stop now, gotta go find some dinner...

greg k-h

[toc] | [prev] | [next] | [standalone]

#1659034

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Tuesday, June 6, 2017 11:06 AM
> To: Kershner, David A <David.Kershner@unisys.com>
> Cc: corbet@lwn.net; tglx@linutronix.de; mingo@kernel.org; akpm@linux-
> foundation.org; jes.sorensen@gmail.com; linux-kernel@vger.kernel.org;
> linux-doc@vger.kernel.org; driverdev-devel@linuxdriverproject.org; *S-Par-
> Maintainer <SParMaintainer@unisys.com>
> Subject: Re: [PATCH 0/3] move visorbus out of staging to
> drivers/virt/visorbus
> 
> On Tue, Jun 06, 2017 at 04:54:30PM +0200, Greg KH wrote:
> > On Tue, Jun 06, 2017 at 04:53:22PM +0200, Greg KH wrote:
> > > On Tue, Jun 06, 2017 at 04:49:09PM +0200, Greg KH wrote:
> > > > On Mon, Jun 05, 2017 at 04:07:29PM -0400, David Kershner wrote:
> > > > > This patchset moves drivers/staging/unisys/include to
> > > > > include/linux/visorbus, and moves drivers/staging/unisys/visorbus to
> > > > > drivers/virt/visorbus.
> > > >
> > > > Um, are you thinking it is ready to be moved?  Have you asked for
> > > > another review?
> > > >

Thank you for taking a quick look at our patch series. Part of the motivation
behind this submission was, in fact, to initiate another code review. What is
the formal procedure for initiating a code review?

> > > > In a totally random chance, I was doing some driver core work today
> and
> > > > I noticed that in drivers/staging/unisys/visorbus/visorbus_main.c, you
> > > > have 2 tabs for your 'struct attribute' variables, which is really odd.
> > > >

Sorry I missed that; I guess my eyes glazed over by the time I got to that file,
and I was expecting checkpatch to catch that. Now I know better, and I will be
looking for more things. Thanks for catching.

> > > > Also, you should be using the ATTRIBUTE_GROUPS() macro for them
> instead
> > > > of having to "open code" the struct attribute_group lists.
> > > >
> > > > So either you all have horrible luck in that I just happened to find the
> > > > only remaining problem, or that you should proabably ask for a good
> code
> > > > audit, I haven't looked at the code before today since the last round of
> > > > "fun" I found in just one other random file :)
> > >
> > > Also, many of the attribute callbacks in that file seem to all have
> > > their leading '{' in the wrong place.  Odd that checkpatch.pl doesn't
> > > catch that...
> > >
> > > partition_handle_show() is one such example that is obviously wrong.
> > >
> > > There's also one checkpatch.pl warning for it, which should probably be
> > > resolved as well.
> >
> > drivers/staging/unisys/visorbus/visorbus_main.c:1035: WARNING: Prefer
> using '"%s...", __func__' to using 'create_bus_instance', this function's name,
> in a string
> >
> > to be specific, something you should have caught, right?
> >
> > Are you sure this is ready to be moved out of staging?  :(
> 
> Eek, I can't look away...
> 
> You do this a bunch:
> 	if (dev->visorchannel) {
> 		visorchannel_destroy(dev->visorchannel);
> 
> yet the first thing that visorchannel_destroy() does is check for null.
> So, no need to test this twice, right, only do so in the function, that
> will make your code flow a lot "smoother" where ever you are calling
> this.
> 
> Ok, I'll stop now, gotta go find some dinner...
> 

We will do some more internal reviews, and send out fixes for things
we find. I hope you enjoyed your dinner.

> greg k-h

[toc] | [prev] | [next] | [standalone]

#1659056

On Tue, Jun 06, 2017 at 06:20:17PM +0000, Kershner, David A wrote:
> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Tuesday, June 6, 2017 11:06 AM
> > To: Kershner, David A <David.Kershner@unisys.com>
> > Cc: corbet@lwn.net; tglx@linutronix.de; mingo@kernel.org; akpm@linux-
> > foundation.org; jes.sorensen@gmail.com; linux-kernel@vger.kernel.org;
> > linux-doc@vger.kernel.org; driverdev-devel@linuxdriverproject.org; *S-Par-
> > Maintainer <SParMaintainer@unisys.com>
> > Subject: Re: [PATCH 0/3] move visorbus out of staging to
> > drivers/virt/visorbus
> > 
> > On Tue, Jun 06, 2017 at 04:54:30PM +0200, Greg KH wrote:
> > > On Tue, Jun 06, 2017 at 04:53:22PM +0200, Greg KH wrote:
> > > > On Tue, Jun 06, 2017 at 04:49:09PM +0200, Greg KH wrote:
> > > > > On Mon, Jun 05, 2017 at 04:07:29PM -0400, David Kershner wrote:
> > > > > > This patchset moves drivers/staging/unisys/include to
> > > > > > include/linux/visorbus, and moves drivers/staging/unisys/visorbus to
> > > > > > drivers/virt/visorbus.
> > > > >
> > > > > Um, are you thinking it is ready to be moved?  Have you asked for
> > > > > another review?
> > > > >
> 
> Thank you for taking a quick look at our patch series. Part of the motivation
> behind this submission was, in fact, to initiate another code review. What is
> the formal procedure for initiating a code review?

Send an email that says, "Hey Greg, we think the code is ready to be
moved out of staging, can you review it to see if we have missed
anything?"

Of course, do it _AFTER_ you have fixed up the checkpatch.pl issues.
That's not even done yet...

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1658849

On Tue, Jun 06, 2017 at 04:49:09PM +0200, Greg KH wrote:
> On Mon, Jun 05, 2017 at 04:07:29PM -0400, David Kershner wrote:
> > This patchset moves drivers/staging/unisys/include to
> > include/linux/visorbus, and moves drivers/staging/unisys/visorbus to
> > drivers/virt/visorbus.
> 
> Um, are you thinking it is ready to be moved?  Have you asked for
> another review?
> 
> In a totally random chance, I was doing some driver core work today and
> I noticed that in drivers/staging/unisys/visorbus/visorbus_main.c, you
> have 2 tabs for your 'struct attribute' variables, which is really odd.
> 
> Also, you should be using the ATTRIBUTE_GROUPS() macro for them instead
> of having to "open code" the struct attribute_group lists.
> 
> So either you all have horrible luck in that I just happened to find the
> only remaining problem, or that you should proabably ask for a good code
> audit, I haven't looked at the code before today since the last round of
> "fun" I found in just one other random file :)

Also, many of the attribute callbacks in that file seem to all have
their leading '{' in the wrong place.  Odd that checkpatch.pl doesn't
catch that...

partition_handle_show() is one such example that is obviously wrong.

There's also one checkpatch.pl warning for it, which should probably be
resolved as well.

I strongly suggest you go through everything again on your own...

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1658871 — Re: [PATCH 0/3] move visorbus out of staging to drivers/virt/visorbus

On Tue, 2017-06-06 at 16:53 +0200, Greg KH wrote:
> On Tue, Jun 06, 2017 at 04:49:09PM +0200, Greg KH wrote:
> > I noticed that in drivers/staging/unisys/visorbus/visorbus_main.c, you
> > have 2 tabs for your 'struct attribute' variables, which is really odd.
[]
> Also, many of the attribute callbacks in that file seem to all have
> their leading '{' in the wrong place.  Odd that checkpatch.pl doesn't
> catch that...

checkpatch doesn't really check much about inconsistent
indentation.  I believe the only new statement indentation
check is after an if.

For instance, checkpatch doesn't emit a warning on this code:

struct foo {
		int bar;
};

struct foo *alloc_foo(void)
{
			struct foo *baz = malloc(sizeof(struct foo));
		if (baz)
			baz->bar = 1;
				return baz;
}

[toc] | [prev] | [next] | [standalone]

#1658875

On Tue, Jun 06, 2017 at 08:33:49AM -0700, Joe Perches wrote:
> On Tue, 2017-06-06 at 16:53 +0200, Greg KH wrote:
> > On Tue, Jun 06, 2017 at 04:49:09PM +0200, Greg KH wrote:
> > > I noticed that in drivers/staging/unisys/visorbus/visorbus_main.c, you
> > > have 2 tabs for your 'struct attribute' variables, which is really odd.
> []
> > Also, many of the attribute callbacks in that file seem to all have
> > their leading '{' in the wrong place.  Odd that checkpatch.pl doesn't
> > catch that...
> 
> checkpatch doesn't really check much about inconsistent
> indentation.  I believe the only new statement indentation
> check is after an if.
> 
> For instance, checkpatch doesn't emit a warning on this code:
> 
> struct foo {
> 		int bar;
> };
> 
> struct foo *alloc_foo(void)
> {
> 			struct foo *baz = malloc(sizeof(struct foo));
> 		if (baz)
> 			baz->bar = 1;
> 				return baz;
> }

Ok, but the following code in that file should be caught, right:

static ssize_t partition_handle_show(struct device *dev,
                                     struct device_attribute *attr,
                                     char *buf) {
        struct visor_device *vdev = to_visor_device(dev);
        u64 handle = visorchannel_get_clientpartition(vdev->visorchannel);

        return sprintf(buf, "0x%llx\n", handle);
}
static DEVICE_ATTR_RO(partition_handle);


The initial { is in the wrong place...

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1658884 — Re: [PATCH 0/3] move visorbus out of staging to drivers/virt/visorbus

On Tue, 2017-06-06 at 17:39 +0200, Greg KH wrote:
> On Tue, Jun 06, 2017 at 08:33:49AM -0700, Joe Perches wrote:
> > On Tue, 2017-06-06 at 16:53 +0200, Greg KH wrote:
> > > On Tue, Jun 06, 2017 at 04:49:09PM +0200, Greg KH wrote:
> > > > I noticed that in drivers/staging/unisys/visorbus/visorbus_main.c, you
> > > > have 2 tabs for your 'struct attribute' variables, which is really odd.
> > 
> > []
> > > Also, many of the attribute callbacks in that file seem to all have
> > > their leading '{' in the wrong place.  Odd that checkpatch.pl doesn't
> > > catch that...
[]
> the following code in that file should be caught, right:
> 
> static ssize_t partition_handle_show(struct device *dev,
>                                      struct device_attribute *attr,
>                                      char *buf) {
>         struct visor_device *vdev = to_visor_device(dev);
>         u64 handle = visorchannel_get_clientpartition(vdev->visorchannel);
> 
>         return sprintf(buf, "0x%llx\n", handle);
> }
> static DEVICE_ATTR_RO(partition_handle);

Not really.

> The initial { is in the wrong place...

True.

Please understand that checkpatch looks at patches one line
at a time.  It's not very smart about function definitions
or context.

checkpatch's function definition code is pretty limited.
It can miss a lot of style misuses.

Single line function definitions brace tests work well.
Multiple line function definitions do not.

[toc] | [prev] | [next] | [standalone]

#1658894

On Tue, Jun 06, 2017 at 08:52:27AM -0700, Joe Perches wrote:
> On Tue, 2017-06-06 at 17:39 +0200, Greg KH wrote:
> > On Tue, Jun 06, 2017 at 08:33:49AM -0700, Joe Perches wrote:
> > > On Tue, 2017-06-06 at 16:53 +0200, Greg KH wrote:
> > > > On Tue, Jun 06, 2017 at 04:49:09PM +0200, Greg KH wrote:
> > > > > I noticed that in drivers/staging/unisys/visorbus/visorbus_main.c, you
> > > > > have 2 tabs for your 'struct attribute' variables, which is really odd.
> > > 
> > > []
> > > > Also, many of the attribute callbacks in that file seem to all have
> > > > their leading '{' in the wrong place.  Odd that checkpatch.pl doesn't
> > > > catch that...
> []
> > the following code in that file should be caught, right:
> > 
> > static ssize_t partition_handle_show(struct device *dev,
> >                                      struct device_attribute *attr,
> >                                      char *buf) {
> >         struct visor_device *vdev = to_visor_device(dev);
> >         u64 handle = visorchannel_get_clientpartition(vdev->visorchannel);
> > 
> >         return sprintf(buf, "0x%llx\n", handle);
> > }
> > static DEVICE_ATTR_RO(partition_handle);
> 
> Not really.
> 
> > The initial { is in the wrong place...
> 
> True.
> 
> Please understand that checkpatch looks at patches one line
> at a time.  It's not very smart about function definitions
> or context.
> 
> checkpatch's function definition code is pretty limited.
> It can miss a lot of style misuses.
> 
> Single line function definitions brace tests work well.
> Multiple line function definitions do not.

Ok, that makes sense why this is missed.  No big deal, a simple visual
inspection shows stuff like this up really easily, which obviously no
one did yet on this file :)

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1659835

On Tue, Jun 06, 2017 at 04:49:09PM +0200, Greg KH wrote:
> On Mon, Jun 05, 2017 at 04:07:29PM -0400, David Kershner wrote:
> > This patchset moves drivers/staging/unisys/include to
> > include/linux/visorbus, and moves drivers/staging/unisys/visorbus to
> > drivers/virt/visorbus.
> 
> Um, are you thinking it is ready to be moved?  Have you asked for
> another review?
> 
> In a totally random chance, I was doing some driver core work today and
> I noticed that in drivers/staging/unisys/visorbus/visorbus_main.c, you
> have 2 tabs for your 'struct attribute' variables, which is really odd.

That's the least of the problems.  Just about any function in there is
doing functionally stupid things.  E.g. raw sg_phys() calls for I/O
instead of dma mapping routines or parsing SCSI INQUIRY data in the
driver.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web