> Michael Zoran <mzoran@xxxxxxxxxxxx> hat am 2. März 2017 um 04:41 geschrieben:
>
>
> This patch adds compatibility wrappers for the ioctls
> exposed by vchiq/vc04_services. The compat ioctls are
> completely implemented on top of the native ioctls. No
> existing lines are modified.
>
> While the ideal approach would be to cleanup the existing
> code, this path is simplier and easier to review. While
> it does have a small runtime performance penality vs
> seperating the existing code into wrapper+worker functions,
> the penality is small since only the metadata is copied
> back onto the 32 bit user mode stack.
>
> The on top of approach is the approach used by several
> existing performance critical subsystems of Linux such
> as the DRM 3D graphics subsystem.
>
> Testing:
>
> 1. A 32 bit chroot was created on a RPI 3 and vchiq_test
> was built for armhf. The usual tests were run such as
> vchiq_test -f 10 and vchiq_test -p.
>
> 2. This patch was applied to the shipping version of
> the Linux kernel used for the RPI and that kernel was
> built for arm64. That kernel was used to boot Raspbian.
> Many of the builtin features are now functional such
> as the "hello_pi" examples, and minecraft_pi.
>
> Changes:
> V1 - Complete rewrite of the ioctl code.
> V2 - Rewrite of only ioctls that change
> between 32 bit and 64 bit.
> V3 - Minor changes.
> V4(This Version) - Abandon cleaning up the
> exising code and completely write the
> wrappers on top of the native ioctls.
> No existing lines are changed.
>
> Signed-off-by: Michael Zoran <mzoran@xxxxxxxxxxxx>
> ---
> .../vc04_services/interface/vchiq_arm/vchiq_arm.c | 446 +++++++++++++++++++++
> 1 file changed, 446 insertions(+)
>
> 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 19bd4ac6e855..90dfa79089d3 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"
> @@ -1227,6 +1228,448 @@ vchiq_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> return ret;
> }
>
> +#if defined(CONFIG_COMPAT)
> +
> +struct vchiq_service_params32 {
> + int fourcc;
> + compat_uptr_t callback;
> + compat_uptr_t userdata;
> + short version; /* Increment for non-trivial changes */
> + short version_min; /* Update for incompatible changes */
> +};
> +
> +struct vchiq_create_service32 {
> + struct vchiq_service_params32 params;
> + int is_open;
> + int is_vchi;
> + unsigned int handle; /* OUT */
> +};
> +
> +#define VCHIQ_IOC_CREATE_SERVICE32 \
> + _IOWR(VCHIQ_IOC_MAGIC, 2, struct vchiq_create_service32)
> +
> +static long
> +vchiq_compat_ioctl_create_service(
> + struct file *file,
> + unsigned int cmd,
> + unsigned long arg)
> +{
> + VCHIQ_CREATE_SERVICE_T __user *args;
> + struct vchiq_create_service32 __user *ptrargs32 =
> + (struct vchiq_create_service32 __user *)arg;
> + struct vchiq_create_service32 args32;
> + long ret;
> +
> + args = compat_alloc_user_space(sizeof(*args));
> + if (!args)
> + return -EFAULT;
> +
> + if (copy_from_user(&args32,
> + (struct vchiq_create_service32 __user *)arg,
> + sizeof(args32)))
> + return -EFAULT;
> +
> + if (put_user(args32.params.fourcc, &args->params.fourcc) ||
> + put_user(compat_ptr(args32.params.callback),
> + &args->params.callback) ||
> + put_user(compat_ptr(args32.params.userdata),
> + &args->params.userdata) ||
> + put_user(args32.params.version, &args->params.version) ||
> + put_user(args32.params.version_min,
> + &args->params.version_min) ||
> + put_user(args32.is_open, &args->is_open) ||
> + put_user(args32.is_vchi, &args->is_vchi) ||
> + put_user(args32.handle, &args->handle))
> + return -EFAULT;
> +
> + ret = vchiq_ioctl(file, VCHIQ_IOC_CREATE_SERVICE, (unsigned long)args);
> +
> + if (ret < 0)
> + return ret;
Within your patch the zero value is sometimes handled as an error and sometimes not. Please make it consistent.
> +
> + if (get_user(args32.handle, &args->handle))
> + return -EFAULT;
> +
> + if (copy_to_user(&ptrargs32->handle,
> + &args32.handle,
> + sizeof(args32.handle)))
> + return -EFAULT;
There are several places in this patch where there is a break for each parameter. We should try to fit it in one line where possible.
> +
> ...
> +}
> +
> +struct vchiq_dump_mem32 {
> + compat_uptr_t virt_addr;
> + u32 num_bytes;
> +};
> +
> +#define VCHIQ_IOC_DUMP_PHYS_MEM32 \
> + _IOW(VCHIQ_IOC_MAGIC, 15, struct vchiq_dump_mem32)
> +
> +static long
> +vchiq_compat_ioctl_dump_phys_mem(struct file *file,
> + unsigned int cmd,
> + unsigned long arg)
> +{
> + VCHIQ_DUMP_MEM_T *args;
> + struct vchiq_dump_mem32 args32;
> +
> + args = compat_alloc_user_space(sizeof(*args));
> + if (!args)
> + return -EFAULT;
> +
> + if (copy_from_user(&args32,
> + (struct vchiq_dump_mem32 *)arg,
> + sizeof(args32)))
> + return -EFAULT;
> +
> + if (put_user(compat_ptr(args32.virt_addr), &args->virt_addr) ||
> + put_user(args32.num_bytes, &args->num_bytes))
> + return -EFAULT;
> +
> + return vchiq_ioctl(file, VCHIQ_IOC_DUMP_PHYS_MEM, (unsigned long)args);
> +}
Do we have any user of this ioctl? I had a better feeling if we could make this a no-op.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
