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

[Michael Thayer]

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

Please be clear, I am not trying to dictate to anyone.  The code is GPL,
it is fine to include it with whatever modifications you deem
appropriate.  Since individual distributions are already doing that, it
will still simplify our life somewhat if it is in the upstream kernel,
in whatever form, rather than in multiple forks.

> 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.  I am assuming here that we are looking for
the best way to do something which will be useful to a lot of people.

Regards
Michael

> good luck!
> 
> greg k-h
> 
-- 
Michael Thayer | VirtualBox engineer
ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt

ORACLE Deutschland B.V. & Co. KG
Hauptverwaltung: Riesstraße 25, D-80992 München
Registergericht: Amtsgericht München, HRA 95603

Komplementärin: ORACLE Deutschland Verwaltung B.V.
Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister
der Handelskammer Midden-Nederland, Nr. 30143697
Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher