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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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.

Remember, this code needs us, we don't need this code at all :)

good luck!

greg k-h
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel