From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752061AbcERWN4 (ORCPT ); Wed, 18 May 2016 18:13:56 -0400 Received: from www.linutronix.de ([62.245.132.108]:44257 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751085AbcERWNy (ORCPT ); Wed, 18 May 2016 18:13:54 -0400 Date: Thu, 19 May 2016 00:11:46 +0200 (CEST) From: Thomas Gleixner To: "Sell, Timothy C" cc: "mingo@kernel.org" , "dave.hansen@linux.intel.com" , "timur@freescale.com" , "galak@kernel.crashing.org" , "Kershner, David A" , "corbet@lwn.net" , "mingo@redhat.com" , "hpa@zytor.com" , "Arfvidson, Erik" , "hofrat@osadl.org" , "dzickus@redhat.com" , "Curtin, Alexander Paul" , "janani.rvchndrn@gmail.com" , "sudipm.mukherjee@gmail.com" , "prarit@redhat.com" , "Binder, David Anthony" , "nhorman@redhat.com" , "dan.j.williams@intel.com" , "linux-kernel@vger.kernel.org" , "linux-doc@vger.kernel.org" , "driverdev-devel@linuxdriverproject.org" , *S-Par-Maintainer , Greg KH , Jes Sorensen Subject: Re: new driver for drivers/virt/? In-Reply-To: Message-ID: References: User-Agent: Alpine 2.11 (DEB 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 18 May 2016, Sell, Timothy C wrote: > We have a bus driver currently in drivers/staging/unisys/visorbus/ that > we are trying to get out of staging and into the kernel proper. Since > "visorbus" is a driver to host a virtual bus presented to a Linux guest > in a hypervisor environment (refer to > drivers/staging/unisys/Documentation/overview.txt for more details), > Greg KH and Jes Sorensen have suggested the possibility that drivers/virt/ > might be a good place for visorbus. But right now, we see that the only > driver under drivers/virt/ is the Freescale hypervisor environment, which > made us wonder whether this was really the correct place. > > Would you have any guidance for us? > Our intent is to push our visorbus out of staging immediately following > the current merge window. What's the problem with Gregs and Jes suggestion? I don't see any. There is bigger fish to fry than the final place of this driver. I had just a peek at the staging code and there is enough stuff which wants to be cleaned up before moving anywhere. I don't have time to do a proper review now, but here are a few hints upfront: 1) Locking: visordriver_callback_lock: That should be a mutex, not a semaphore periodic_work->lock: Why is this a rw_lock if it's only locked with write_lock? And what's the purpose of this lock at all? 2) Memory barriers: Completely undocumented wmb()s without corresponding rmb()s to do obscure protection of that periodic work stuff. 3) periodic_work: That set of functions is obscure. Especially visor_periodic_work_stop() makes me shudder. See also #2. That work->lock does not inspire my confidence further. 4) Exports: A gazillion of exports which are just wrappers around another set of exports 5) Function comments: Try to mimic kerneldoc comments, i.e. start with: /** but do not implement any of the kerneldoc requirements. I'll try do find a time slot for a proper review of that thing, but don't expect that to happen in the next days. Thanks, tglx