> -----Original Message----- > From: Greg KH [mailto:gregkh@xxxxxxxxxxxxxxxxxxx] > Sent: Tuesday, June 6, 2017 11:06 AM > To: Kershner, David A <David.Kershner@xxxxxxxxxx> > Cc: corbet@xxxxxxx; tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx; akpm@linux- > foundation.org; jes.sorensen@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > linux-doc@xxxxxxxxxxxxxxx; driverdev-devel@xxxxxxxxxxxxxxxxxxxxxx; *S-Par- > Maintainer <SParMaintainer@xxxxxxxxxx> > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
