[PATCH] staging: vc04_services: rework ioctl code path

[Michael Zoran]

On Wed, 2016-11-09 at 22:07 +0100, Arnd Bergmann wrote:
> On Wednesday, November 9, 2016 12:37:27 PM CET Michael Zoran wrote:
> 
> >  static long
> > -vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long
> > arg)
> > +vchiq_ioctl_shutdown(VCHIQ_INSTANCE_T instance,
> > +		     unsigned int cmd,
> > +		     unsigned long arg) {
> 
> This does not use cmd or arg, so you can just drop both parameters.
> In cases where the argument is actually used, better make it take
> a pointer than an unsigned long argument, to save a conversion.
> 
> > +	vchiq_log_trace(vchiq_arm_log_level,
> > +			"vchiq_ioctl(compat) - instance %pK, cmd
> > %s, arg %lx",
> > +			instance,
> > +			((_IOC_TYPE(cmd) == VCHIQ_IOC_MAGIC) &&
> > +			(ioctl_nr <= VCHIQ_IOC_MAX)) ?
> > +			ioctl_names[ioctl_nr] : "<invalid>", arg);
> 
> I'd suggest dropping the log_trace here.
> 
> > +	if ((ioctl_nr > VCHIQ_IOC_MAX) ||
> > +	    (vchiq_ioctl_compat_table[ioctl_nr].ioctl != cmd)) {
> > +		ret = -ENOTTY;
> > +	} else {
> > +		ret =
> > vchiq_ioctl_compat_table[ioctl_nr].func(instance, cmd, arg);
> >  	}
> 
> It's rather unusual to have a table like this, most drivers have a
> simple switch/case statement. If you do a swapper like this, better
> make it do something more and let it also pass the data as a kernel
> pointer that you copy in and out of the kernel according to the
> direction and size bits in the command number.
> 

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