[PATCH] staging: vc04_services: rework ioctl code path

[Michael Zoran]

On Wed, 2016-11-09 at 14:16 -0800, Michael Zoran wrote:
> On Wed, 2016-11-09 at 22:07 +0100, Arnd Bergmann wrote:
> > 
> 
> Since the driver has 17 ioctls, I was thinking it's better to use a
> jump table then a switch.  I modeled this on the gpu/drm
> framework.  It
> also has a large number of ioctls and it has a table like that.
> 
> The reason vchiq_ioctl_shutdown has unused args is do to the jump
> table
> functions all needing the same prototype.
> 
> I can certainly extend the common code and as you say it would remove
> some of the need for hand generated code in each function by doing
> some
> of the copying of data into and out of the kernel. 
> 
> In fact, I was considering moving common error unwind code into the
> command dispatch function to make the code less error prone. 
> 
> Actually, I really, really hate ioctls in general.  They are a mess
> to
> write and are a mess to maintain.  It's too bad that some of the
> wrapper goo for ioctls couldn't be generated through automation kind
> of
> like how people have written compiler like tools for RPC.
> 
> > > @@ -104,6 +109,12 @@ typedef struct vchiq_service_base_struct {
> > >  	void *userdata;
> > >  } VCHIQ_SERVICE_BASE_T;
> > >  
> > > +struct vchiq_service_base32 {
> > > +	int fourcc;
> > > +	u32 callback;
> > > +	u32 userdata;
> > > +};
> > 
> > Maybe better use compat_uptr_t along with compat_ptr() for passing
> > 32-bit pointers. This will however require moving the struct
> > definitions
> > into an #ifdef.
> > 
> > 	Arnd
> 
> That I have absolutely no problem changing.  I'll go ahead and change
> it and resubmit the patch.
> > 

Actually, I'm looking at this a bit more and I'm noticing some
interesting things.

If something like compat_ptr is used, why can the ioctl.h header be
included twice with different #defines to generate both the 32 bit and
64 bit cases.

Also, I'm think more about the kernel pointer idea.  One complication
I'm thinking is of the case of IN/OUT data.   How does it unwind the
operation on a failure of the final copy back to userland?

On thing I'm just brainstorming about is what can't it "lock" the
page(s) for the userland pointer in ram until the end of the ioctl so
that the copy back from the kernel can't ever or can't very easily
fail?