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

[Arnd Bergmann]

On Tuesday, November 8, 2016 2:43:28 PM CET Michael Zoran wrote:
> 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.

Ok

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

Ok, good to know. I'd actually consider 10,000 lines rather large
for a single driver, but that's always a matter of perspective.

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

Ok. The ioctls should get some proper review though.

> Some of the userland tool are closed source as
> well.  For example, I was unable to find the source to vcdbg.

Ouch. Note that generally the approach in the kernel is to only
support kernel/user interfaces that have an open source user
space implementation. If there are commands that are only used
by vcdbg (I have no idea what that is), that means we would
either remove those commands or find someone to reimplement
the tool.

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

This reminds me: we should add a TODO item for the driver to
remove all the semaphores and replace them with completions
or mutexes as appropriate.

	Arnd