Re: [Linaro-mm-sig] [PATCHv2 3/4] staging: android: ion: Add an ioctl for ABI checking

[Arnd Bergmann <arnd@arndb.de>]

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