On Thursday, September 1, 2016 3:40:43 PM CEST Laura Abbott wrote:
> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -22,6 +22,29 @@
> #include "ion_priv.h"
> #include "compat_ion.h"
>
> +union ion_ioctl_arg {
> + struct ion_fd_data fd;
> + struct ion_allocation_data allocation;
> + struct ion_handle_data handle;
> + struct ion_custom_data custom;
> + struct ion_abi_version abi_version;
> +};
Are you introducing this, or just clarifying the defintion of the
existing interface. For new interfaces, we should not have a union
as an ioctl argument. Instead each ioctl command should have one
specific structure (or better a scalar argument).
> +static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
> +{
> + int ret = 0;
> +
> + switch (cmd) {
> + case ION_IOC_ABI_VERSION:
> + ret = arg->abi_version.reserved != 0;
> + break;
> + default:
> + break;
> + }
> +
> + return ret ? -EINVAL : 0;
> +}
I agree with Greg, ioctl interfaces should normally not be versioned,
the usual way is to try a command and see if it fails or not.
> +/**
> + * struct ion_abi_version
> + *
> + * @version - current ABI version
> + */
> +
> +#define ION_ABI_VERSION KERNEL_VERSION(0, 1, 0)
> +
> +struct ion_abi_version {
> + __u32 abi_version;
> + __u32 reserved;
> +};
> +
This interface doesn't really need a "reserved" field, you could
as well use a __u32 by itself. If you ever need a second field,
just add a new command number.
Arnd
