[PATCH] staging: vc04_services: Add 32-bit compatibility ioctls

[Michael Zoran]

On Tue, 2016-11-08 at 23:30 +0100, Arnd Bergmann wrote:
> On Tuesday, November 8, 2016 1:42:03 PM CET Michael Zoran wrote:
> > > 
> > > Another way of doing compat ioctls that doesn't rewrite your
> > > whole
> > > non-compat ioctl path is in drivers/gpu/drm/drm_ioc32.c: the
> > > compat
> > > path
> > > just rewrites the 32-bit ioctl into a 64-bit struct that it
> > > passes to
> > > the normal ioctl handler.
> > 
> > That really is a cool way to do 32-bit compat. I didn't know it was
> > possible to do it that way.
> 
> It's not that nice either: since the native handler expects a user
> pointer, you either have to use compat_alloc_user_space() and copy
> the arguments back to the user space stack, or use set_fs(KERNEL_DS)
> to allow fetching user pointers from kernel memory. Both of these
> are problematic, especially with nested pointers.

That's why I'm thinking it's better to break it up into helper
functions rather then take that route.

> > In this case, the native ioctl mess is so bad that IMHO it's not
> > maintainable. Perhaps it is better to fix the mess now while the
> > whole
> > driver is still marked "broken" and most of the patches are still
> > in
> > linux-next.  
> 
> What is the long-term plan for this driver anyway? Is it going to
> be replaced with another driver to talk to the firmware, or cleaned
> up to the point of graduating out of staging?
> 
> In the second case, we can discuss cleaning up the ioctl API to
> make the calls entirely compatible between 32-bit and 64-bit
> user space at the cost of breaking compatibility with existing
> user space binaries. This mainly involves using fixed-size members
> for everything, and in particular passing pointers through __u64
> variables.
> 
> 	Arnd

I would like to know the answer to that myself actually!  

Since the firmware is distributed independently and from my
understanding it's generally closed source, it's going to be very
difficult to hit the "reset" button and start from scratch.

The driver is actually fixable since it's only 10,000 lines of code and
I would say 50% probably isn't even being used so some of it can be
deleted.

IMHO I would say keeping backward compatibility isn't that bad.  The
driver only has 17 ioctls and a good amount of them don't need a
compatibility layer.  Some of the userland tool are closed source as
well.  For example, I was unable to find the source to vcdbg.

It's just whoever wrote this mess didn't take code structure and
maintainability very seriously!