> -----Original Message----- > From: Sell, Timothy C > Sent: Wednesday, May 18, 2016 6:25 PM > To: 'Thomas Gleixner' > Cc: mingo@xxxxxxxxxx; dave.hansen@xxxxxxxxxxxxxxx; > timur@xxxxxxxxxxxxx; galak@xxxxxxxxxxxxxxxxxxx; Kershner, David A; > corbet@xxxxxxx; mingo@xxxxxxxxxx; hpa@xxxxxxxxx; Arfvidson, Erik; > hofrat@xxxxxxxxx; dzickus@xxxxxxxxxx; Curtin, Alexander Paul; > janani.rvchndrn@xxxxxxxxx; sudipm.mukherjee@xxxxxxxxx; > prarit@xxxxxxxxxx; Binder, David Anthony; nhorman@xxxxxxxxxx; > dan.j.williams@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux- > doc@xxxxxxxxxxxxxxx; driverdev-devel@xxxxxxxxxxxxxxxxxxxxxx; *S-Par- > Maintainer; Greg KH; Jes Sorensen > Subject: RE: new driver for drivers/virt/? > > > -----Original Message----- > > From: Thomas Gleixner [mailto:tglx@xxxxxxxxxxxxx] > > Sent: Wednesday, May 18, 2016 6:12 PM > > To: Sell, Timothy C > > Cc: mingo@xxxxxxxxxx; dave.hansen@xxxxxxxxxxxxxxx; > > timur@xxxxxxxxxxxxx; galak@xxxxxxxxxxxxxxxxxxx; Kershner, David A; > > corbet@xxxxxxx; mingo@xxxxxxxxxx; hpa@xxxxxxxxx; Arfvidson, Erik; > > hofrat@xxxxxxxxx; dzickus@xxxxxxxxxx; Curtin, Alexander Paul; > > janani.rvchndrn@xxxxxxxxx; sudipm.mukherjee@xxxxxxxxx; > > prarit@xxxxxxxxxx; Binder, David Anthony; nhorman@xxxxxxxxxx; > > dan.j.williams@xxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux- > > doc@xxxxxxxxxxxxxxx; driverdev-devel@xxxxxxxxxxxxxxxxxxxxxx; *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 > > _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
