lkml.org 
[lkml]   [2016]   [May]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: new driver for drivers/virt/?
Date
> -----Original Message-----
> From: Thomas Gleixner [mailto:tglx@linutronix.de]
> Sent: Wednesday, May 18, 2016 6:12 PM
> 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/?
>
> 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.
>
> 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
>

\
 
 \ /
  Last update: 2016-05-19 00:41    [W:0.075 / U:0.420 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site