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@xxxxxxxxxxxx>
> ---
> .../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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
