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

[Hans de Goede]

Hi,

On 12-06-17 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 :(

Nope, I removed all that already, unless you count things like:

#define Assert(a) WARN_ON_ONCE(!(a))

Which are compatibility defines to get the osindependent code to
build using Linux native constructs, rather then implementing
a driver specific Assert function.

> 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

The driver specific malloc/free stuff is code to prepare an IPC
packet, that code *used* to call a custom alloc routine, but that
has been replaced with a genalloc.h functions. Maybe the functions
need to be renamed as they do a lot more then just alloc mem
(they also fill the header of the IPC packet). But really there
are no custom malloc / assert / other-generic helper functions
left I removed all those.

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

There is no more 43000 lines of OS abstraction layer which used to
be there, what is left is osindependent/VBoxVideoIPRT.h which offers
the osindepdent code things like Assert() which it needs, and is
only 137 lines.

Anyways I'm fine with cleaning this up, but I did the split at request
of VirtualBox upstream, so I will let Michael weigh in here. Michael,
are you ok with going all the way and cleaning up the few files still
left under the osindependent dir ?

Regards,

Hans