From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751743AbdFFPGZ (ORCPT ); Tue, 6 Jun 2017 11:06:25 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:35414 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751428AbdFFPGX (ORCPT ); Tue, 6 Jun 2017 11:06:23 -0400 Date: Tue, 6 Jun 2017 17:06:15 +0200 From: Greg KH To: David Kershner 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, sparmaintainer@unisys.com Subject: Re: [PATCH 0/3] move visorbus out of staging to drivers/virt/visorbus Message-ID: <20170606150615.GA1337@kroah.com> References: <1496693252-4349-1-git-send-email-david.kershner@unisys.com> <20170606144909.GA31938@kroah.com> <20170606145322.GA32397@kroah.com> <20170606145430.GA32683@kroah.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170606145430.GA32683@kroah.com> User-Agent: Mutt/1.8.3 (2017-05-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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