[PATCH v2] staging: vboxvideo: Add vboxvideo to drivers/staging

[Greg Kroah-Hartman]

On Tue, Jun 13, 2017 at 01:50:16PM +0200, Michael Thayer wrote:
> 12.06.2017 18:03, Greg Kroah-Hartman wrote:
> > On Mon, Jun 12, 2017 at 05:40:21PM +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 12-06-17 13:44, Greg Kroah-Hartman wrote:
> >>> On Mon, Jun 12, 2017 at 12:07:41PM +0200, Hans de Goede wrote:
> >>>>> The most important thing is for the driver to be atomic if it's KMS
> >>>>> only, and it would be good to have someone review that properly.
> >>>>
> >>>> I believe it does not use the atomic APIs atm, so that would be one
> >>>> of the first things to fix then. Another question is if people
> >>>> (you and Daniel at least) can live with the non kernel-coding
> >>>> style shared files under the osindependent dir ?
> >>>
> >>> Why not just spend a few days and fix up all of the kernel-style issues
> >>> so it can be a "real" driver?  It shouldn't take all that long,
> >>> especially for someone with Linux kernel experience (hint, hint...)
> >>
> >> The intention of the stuff below the osindepedent dir is for it to
> >> be shared 1:1 between vboxvideo driver implementations for different
> >> operating-systems. IIRC during the AMD DAL discussion Daniel indicated
> >> that some OS independent code was fine (and would be exempt from coding
> >> style rules) as long as it had a reasonable clean interface and was not
> >> re-implementing anything we already have in the kernel.
> > 
> > In a quick glance at the code in there, there's lots of reimplementing
> > happening :(
> > 
> > Maybe keep the data structures around, but really, you write those once,
> > and then that's it, they should never change, so it shouldn't matter
> > what format they are in.
> > 
> >> If Daniel's verdict is that this needs to be cleaned up, then sure I
> >> can easily do that. As mentioned I already did a lot of cleanup,
> >> including moving all the other files to the kernel coding-style and
> >> removing about 43000 lines of portability cruft / abstraction layers,
> >> what is left under the osindependent directory is just C-structure
> >> definitions and a few small plain C helper functions, which VirtualBox
> >> upstream would like to keep as is...
> > 
> > wrappers for simple things should not be needed at all, come on, you
> > know that.  We don't like driver-specific malloc/free and in/out
> > functions, or asserts, or other crap like that.  This implies that this
> > driver is an island in itself and somehow more "important" than the 12+
> > million other lines of code that it lives within.  Which isn't true.
> > 
> > Just clean it up, that will make it even smaller, which in the end, is
> > what really matters, as that will make it easier to maintain, fix, port
> > to new apis, and everything else.
> > 
> > There's a good reason why we don't have "os abstraction" layers in
> > drivers in Linux, please don't ignore our history and knowledge here for
> > no good reason.
> 
> I would appreciate getting Dave and/or Daniel's feedback here, if they
> have time.  In particular, they might have ideas about how to further
> reduce the abstraction surface.  In the end, the greater the difference
> between the code in the kernel, the harder it is for people to pull
> changes from our code to the kernel, and most of the changes in that
> part are likely to come from our side.  I don't think that converting
> non-Linux-specific code in our tree to kernel style would go down well
> in our team, so if the code as is is not acceptable, the next question
> is, what would be?  If we can come up with something which is alright
> for both modulo a sed script that would still make people's lives
> somewhat easier (though just the different brace style throws a bit of a
> spanner in that).

Once your code is accepted into the main kernel tree, why would you
continue to work in an out-of-tree repo anyway?  That's ripe for
disaster, what's keeping you from just working with the in-tree version?

Note, this isn't anything new.  Hundreds of companies and thousands of
developers do this every year with Linux, they don't have out-of-tree
repos with different coding style rules, as consistent rules are there
for good reason (hint, our brains, you want us to fix your bugs, and we
want you to fix ours.)

> > Remember, this code needs us, we don't need this code at all :)
> 
> I assume that that was not meant that way, but that came over as
> slightly unfriendly to me.

It was meant that way as you are trying to do something that no other
developer group gets away with in the kernel tree, why is this tiny 8k
line driver so special?  I agree it is special to you, that that's
great, but please be aware of the larger picture here.  You will benefit
greatly from being in the kernel source tree (lines removed, bugs fixed,
api changes happen automatically, etc.)  All we ask is that you work on
the same level play ground as everyone else is.  To ask to use a special
coding style and have an os-independant wrapper is not "level" at all,
it insulates you from the rest of the community and prevents anyone else
from helping you out.

I'm not trying to be mean, I'm trying to be honest here.  You do need
us, you already depend on the rest of the kernel source, and developer
community.  To try to act differently is not ok.

> I am assuming here that we are looking for the best way to do
> something which will be useful to a lot of people.

I totally agree, and the reason we have these rules is for us developers
to all work together.  To wall yourself off behind layers like this
within the kernel is not working together at all.

Hans has already done an amazing job stripping out the cruft that was in
the codebase, let's not waste that by preventing that last little bit
from being removed as well (again, smaller codebase, less bugs, more
developers able to help, etc.)

thanks,

greg k-h