linux-kernel - RE: new driver for drivers/virt/?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <fa623f804b444d4d80a3124a25a093d9@US-EXCH13-2.na.uis.unisys.com>
Date:	Thu, 19 May 2016 14:44:27 +0000
From:	"Sell, Timothy C" <Timothy.Sell@...sys.com>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	"mingo@...nel.org" <mingo@...nel.org>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
	"timur@...escale.com" <timur@...escale.com>,
	"galak@...nel.crashing.org" <galak@...nel.crashing.org>,
	"Kershner, David A" <David.Kershner@...sys.com>,
	"corbet@....net" <corbet@....net>,
	"mingo@...hat.com" <mingo@...hat.com>,
	"hpa@...or.com" <hpa@...or.com>,
	"Arfvidson, Erik" <Erik.Arfvidson@...sys.com>,
	"hofrat@...dl.org" <hofrat@...dl.org>,
	"dzickus@...hat.com" <dzickus@...hat.com>,
	"Curtin, Alexander Paul" <Alexander.Curtin@...sys.com>,
	"janani.rvchndrn@...il.com" <janani.rvchndrn@...il.com>,
	"sudipm.mukherjee@...il.com" <sudipm.mukherjee@...il.com>,
	"prarit@...hat.com" <prarit@...hat.com>,
	"Binder, David Anthony" <David.Binder@...sys.com>,
	"nhorman@...hat.com" <nhorman@...hat.com>,
	"dan.j.williams@...el.com" <dan.j.williams@...el.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"driverdev-devel@...uxdriverproject.org" 
	<driverdev-devel@...uxdriverproject.org>,
	*S-Par-Maintainer <SParMaintainer@...sys.com>,
	Greg KH <gregkh@...uxfoundation.org>,
	"Jes Sorensen" <Jes.Sorensen@...hat.com>
Subject: RE: new driver for drivers/virt/?

> -----Original Message-----
> From: Sell, Timothy C
> Sent: Wednesday, May 18, 2016 6:25 PM
> To: 'Thomas Gleixner'
> Cc: mingo@...nel.org; dave.hansen@...ux.intel.com;
> timur@...escale.com; galak@...nel.crashing.org; Kershner, David A;
> corbet@....net; mingo@...hat.com; hpa@...or.com; Arfvidson, Erik;
> hofrat@...dl.org; dzickus@...hat.com; Curtin, Alexander Paul;
> janani.rvchndrn@...il.com; sudipm.mukherjee@...il.com;
> prarit@...hat.com; Binder, David Anthony; nhorman@...hat.com;
> dan.j.williams@...el.com; linux-kernel@...r.kernel.org; linux-
> doc@...r.kernel.org; driverdev-devel@...uxdriverproject.org; *S-Par-
> Maintainer; Greg KH; Jes Sorensen
> Subject: RE: new driver for drivers/virt/?
> 
> > -----Original Message-----
> > From: Thomas Gleixner [mailto:tglx@...utronix.de]
> > Sent: Wednesday, May 18, 2016 6:12 PM
> > To: Sell, Timothy C
> > Cc: mingo@...nel.org; dave.hansen@...ux.intel.com;
> > timur@...escale.com; galak@...nel.crashing.org; Kershner, David A;
> > corbet@....net; mingo@...hat.com; hpa@...or.com; Arfvidson, Erik;
> > hofrat@...dl.org; dzickus@...hat.com; Curtin, Alexander Paul;
> > janani.rvchndrn@...il.com; sudipm.mukherjee@...il.com;
> > prarit@...hat.com; Binder, David Anthony; nhorman@...hat.com;
> > dan.j.williams@...el.com; linux-kernel@...r.kernel.org; linux-
> > doc@...r.kernel.org; driverdev-devel@...uxdriverproject.org; *S-Par-
> > Maintainer; Greg KH; Jes Sorensen
> > Subject: Re: new driver for drivers/virt/?
> >
> > 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.
> >
> 
> That's good; glad you agree with them.  We just wanted to double-check
> with those of you listed as maintainers of drivers/virt/.  Thanks.
> 
> > 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.
> >

tglx:

Re wmb/rmb: I believe you must have been looking at an older
version of our code in Linus' tree, rather than the latest version in
Greg's staging-next tree.  Reason is, Linus' tree only contains our
source code thru 3/11 (i.e., see
(http://git.kernel.org/cgit/linux/kernel/git/stable/linux-stable.git/log/drivers/staging/unisys),
and wmb() was removed in Greg's staging-next tree on 3/30 with
commit 64938182e7836650feeb9b2b9dadd510ed4b0dad
(https://git.kernel.org/cgit/linux/kernel/git/gregkh/staging.git/commit/drivers/staging?h=staging-next&id=64938182e7836650feeb9b2b9dadd510ed4b0dad).

We've made a LOT of changes in Greg's staging-next since 3/11.
However, the other issues you bring up still look to be valid in our
latest source code (in Greg's staging-next).

Tim Sell

> > 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.
> >
> 
> We'll take a look at these.  Thanks.
> 
> Tim Sell
> 
> > 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
> >