new driver for drivers/virt/?

new driver for drivers/virt/?

[Sell, Timothy C]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Wednesday, May 18, 2016 1:27 PM
> To: Jes Sorensen
> Cc: Kershner, David A; corbet@lwn.net; tglx@linutronix.de;
> mingo@redhat.com; hpa@zytor.com; Arfvidson, Erik; Sell, Timothy C;
> 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
> Subject: Re: [PATCH 0/5] add bus driver for Unisys s-Par paravirtualized
> devices to arch/x86
> 
> On Wed, May 18, 2016 at 12:03:58PM -0400, Jes Sorensen wrote:
> > Greg KH <gregkh@linuxfoundation.org> writes:
> > > On Tue, May 17, 2016 at 07:49:53PM -0400, Jes Sorensen wrote:
> > >> > Ok, but still no need to put it under arch/ anything, it should go in
> > >> > drivers/ like all other drivers and busses are, no matter what the arch
> > >> > it happens to run on is.
> > >>
> > >> I don't think thats obvious. arch/x86/kvm is an example of this, Sparc
> > >> and PPC also have their stuff under arch/.
> > >
> > > For some things, yes, but let's not make the same mistakes as others :)
> > >
> > > Look at drivers/hv/ for an example of a very x86-only bus and driver
> > > subsystem living in drivers/  Please don't burry driver stuff in arch/
> > > the ARM developers are trying to fix their mistakes of the past and move
> > > all of their cruft out of arch/ for that reason.
> >
> > Works for me.
> >
> > So should they put it in drivers/visorbus or drivers/bus/visorbus?
> 
> drivers/visorbus please.  drivers/bus/ is an "odd" thing :(
> 
> > What about drivers/virt? Would you suggest hv and xen gets moved in
> > there?
> 
> Ah, yeah, that would be great, never noticed it before, despite being
> there since 2011...
> 
> thanks,
> 
> greg k-h

tglx, Ingo, Dave, Timur, Kumar:

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.

Thanks.

Tim Sell

Re: new driver for drivers/virt/?

[Thomas Gleixner]

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

   

Re: new driver for drivers/virt/?

[Greg KH]

On Thu, May 19, 2016 at 12:11:46AM +0200, Thomas Gleixner wrote:
> 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.

Neither do I, odd that they wanted a third opinion :(

> 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:

<snip>

I don't think anyone has given a good review of the code yet, I know
it's been a long time for me.  It's on my todo list, but that's not
going to happen until way after 4.7-rc1 is out.  Thanks for this initial
list.

greg k-h

RE: new driver for drivers/virt/?

[Sell, Timothy C]

> -----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
> 

RE: new driver for drivers/virt/?

[Sell, Timothy C]

> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Wednesday, May 18, 2016 6:25 PM
> To: Thomas Gleixner
> Cc: Sell, Timothy C; 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; Jes Sorensen
> Subject: Re: new driver for drivers/virt/?
> 
> On Thu, May 19, 2016 at 12:11:46AM +0200, Thomas Gleixner wrote:
> > 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.
> 
> Neither do I, odd that they wanted a third opinion :(

Sorry; I didn't intend to belittle your opinions.  Since we want to
push code to drivers/virt/, I figured it would be prudent to get
permission from those folks reported by:

	./scripts/get_maintainer.pl -f drivers/virt/

first.  That was the intent of my original email, and why I chose
the distribution list I did.

I've never done this before, so I spend quite a lot of time groping
around blindly...   ;-(

Tim Sell

> 
> > 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:
> 
> <snip>
> 
> I don't think anyone has given a good review of the code yet, I know
> it's been a long time for me.  It's on my todo list, but that's not
> going to happen until way after 4.7-rc1 is out.  Thanks for this initial
> list.
> 
> greg k-h

RE: new driver for drivers/virt/?

[Sell, Timothy C]

> -----Original Message-----
> From: Sell, Timothy C
> Sent: Wednesday, May 18, 2016 6:25 PM
> To: 'Thomas Gleixner'
> 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/?
> 
> > -----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.
> >

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
> >