[PATCH V2 1/7] staging: vchiq_arm: Add compat ioctl for create service

[Dan Carpenter]

I'm sorry but I really hate this again...  :/

On Sat, Jan 21, 2017 at 09:48:10AM -0800, Michael Zoran wrote:
> This change is the first in a set of changes to add compat ioctls for vc04_services. 
> In the change set, each ioctl modifed is pulled into a compat and native specific
> wrapper that calls a platform neutral handler function.
> 
> At this time only the ioctls needed for compat are handled, but
> a general pattern is developed that can be used for cleaning up
> the other ioctls.
> 
> This change contains the general framework and the ioctl handler for 
> the create service ioctl.
> 
> Signed-off-by: Michael Zoran <mzoran at crowfest.net>
> ---
>  .../vc04_services/interface/vchiq_arm/vchiq_arm.c  | 360 ++++++++++++++++-----
>  1 file changed, 276 insertions(+), 84 deletions(-)
> 
> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> index 1dc8627e65b0..a5f5d5b6f938 100644
> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_arm.c
> @@ -47,6 +47,7 @@
>  #include <linux/list.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> +#include <linux/compat.h>
>  #include <soc/bcm2835/raspberrypi-firmware.h>
>  
>  #include "vchiq_core.h"
> @@ -506,6 +507,254 @@ vchiq_ioc_queue_message(VCHIQ_SERVICE_HANDLE_T handle,
>  				   &context, total_size);
>  }
>  
> +struct vchiq_ioctl_ctxt;
> +typedef long (*vchiq_ioctl_cpyret_handler_t)(struct vchiq_ioctl_ctxt *ctxt);
> +
> +/*
> + * struct vchiq_ioctl_ctxt
> + *
> + * Holds context information across a single ioctl call from user mode.
> + * This structure is used to reduce the number of parameters passed
> + * to each of the handler functions to process the ioctl.
> + */
> +
> +struct vchiq_ioctl_ctxt {
> +	struct file *file;
> +	unsigned int cmd;
> +	unsigned long arg;
> +	void *args;
> +	VCHIQ_INSTANCE_T instance;
> +	VCHIQ_STATUS_T status;
> +	VCHIQ_SERVICE_T *service;
> +	vchiq_ioctl_cpyret_handler_t cpyret_handler;
> +};
> +

This doesn't simplify anything at all...  It just hides the parameters
and makes things more complicated on the caller and the implementor.

Also "args" is now a void pointer which makes me very sad.  :(

> +typedef long (*vchiq_ioctl_handler_t)(struct vchiq_ioctl_ctxt *ctxt);
> +
> +static long
> +vchiq_map_status(VCHIQ_STATUS_T status)
> +{
> +	switch (status) {
> +	case VCHIQ_SUCCESS:
> +		return 0;
> +	case VCHIQ_ERROR:
> +		return -EIO;
> +	case VCHIQ_RETRY:
> +		return -EINTR;
> +	default:
> +		return -EIO;
> +	}
> +}

I don't really like this either...  We should be getting rid of these
custom error codes not formalizing them.  :(

> +
> +static long
> +vchiq_dispatch_ioctl(vchiq_ioctl_handler_t handler,
> +		     struct file *file, unsigned int cmd, unsigned long arg) {
> +	struct vchiq_ioctl_ctxt ctxt;
> +	long ret = 0;
> +
> +	ctxt.file		= file;
> +	ctxt.cmd		= cmd;
> +	ctxt.arg		= arg;
> +	ctxt.args		= NULL;
> +	ctxt.instance		= file->private_data;
> +	ctxt.status		= VCHIQ_SUCCESS;
> +	ctxt.service		= NULL;
> +	ctxt.cpyret_handler	= NULL;
> +
> +	vchiq_log_trace(vchiq_arm_log_level,
> +			"vchiq_ioctl - instance %pK, cmd %s, arg %lx",
> +			ctxt.instance,
> +			ioctl_names[_IOC_NR(cmd)],
> +			arg);
> +
> +	ret = handler(&ctxt);
> +
> +	if (ctxt.service)
> +		unlock_service(ctxt.service);
> +
> +	if (ret < 0)
> +		vchiq_log_info(vchiq_arm_log_level,
> +			       "  ioctl instance %lx, cmd %s -> status %d, %ld",
> +			       (unsigned long)ctxt.instance,
> +			       ioctl_names[_IOC_NR(cmd)],
> +			       ctxt.status, ret);
> +	else
> +		vchiq_log_trace(vchiq_arm_log_level,
> +				"  ioctl instance %lx, cmd %s -> status %d, %ld",
> +				(unsigned long)ctxt.instance,
> +				ioctl_names[_IOC_NR(cmd)],
> +				ctxt.status, ret);
> +
> +	return ret;

I don't see the point of any of this debugging.  Just leave it out.  Use
ftrace.

> +}
> +
> +static long
> +vchiq_ioctl_do_create_service(struct vchiq_ioctl_ctxt *ctxt)

Finally, we have reached he meat of this patch.  Please, pull this
bit out in patch #1 like I asked.  Then in the next patch implement
the compat ioctl.

I don't like the cpyret function pointers either...

This stuff should really be very simple.  I feel like you're engineering
it to be way harder than it needs to be.  I'm sorry.  :(

regards,
dan carpenter