Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1474966

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

Show all headers | View raw

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

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCHv2 0/4] New Ion ioctls Laura Abbott <labbott@redhat.com> - 2016-09-02 00:50 +0200
  [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps Laura Abbott <labbott@redhat.com> - 2016-09-02 00:50 +0200
    Re: [PATCHv2 4/4] staging: android: ion: Add ioctl to query  available heaps Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-09-02 08:20 +0200
      Re: [PATCHv2 4/4] staging: android: ion: Add ioctl to query available  heaps Laura Abbott <labbott@redhat.com> - 2016-09-02 22:50 +0200
        Re: [PATCHv2 4/4] staging: android: ion: Add ioctl to query  available heaps Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-09-03 15:00 +0200
  [PATCHv2 1/4] staging: android: ion: Drop heap type masks Laura Abbott <labbott@redhat.com> - 2016-09-02 00:50 +0200
    Re: [PATCHv2 1/4] staging: android: ion: Drop heap type masks Brian Starkey <brian.starkey@arm.com> - 2016-09-02 15:50 +0200
      Re: [PATCHv2 1/4] staging: android: ion: Drop heap type masks Laura Abbott <labbott@redhat.com> - 2016-09-02 21:40 +0200
        Re: [PATCHv2 1/4] staging: android: ion: Drop heap type masks Brian Starkey <brian.starkey@arm.com> - 2016-09-05 13:30 +0200
          Re: [PATCHv2 1/4] staging: android: ion: Drop heap type masks Laura Abbott <labbott@redhat.com> - 2016-09-07 00:20 +0200
            Re: [PATCHv2 1/4] staging: android: ion: Drop heap type masks Brian Starkey <brian.starkey@arm.com> - 2016-09-07 11:00 +0200
  [PATCHv2 3/4] staging: android: ion: Add an ioctl for ABI checking Laura Abbott <labbott@redhat.com> - 2016-09-02 00:50 +0200
    Re: [PATCHv2 3/4] staging: android: ion: Add an ioctl for ABI  checking Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-09-02 08:20 +0200
      Re: [PATCHv2 3/4] staging: android: ion: Add an ioctl for ABI  checking Laura Abbott <labbott@redhat.com> - 2016-09-02 22:30 +0200
    Re: [Linaro-mm-sig] [PATCHv2 3/4] staging: android: ion: Add an ioctl for ABI checking Arnd Bergmann <arnd@arndb.de> - 2016-09-02 11:10 +0200
      Re: [Linaro-mm-sig] [PATCHv2 3/4] staging: android: ion: Add an ioctl  for ABI checking Laura Abbott <labbott@redhat.com> - 2016-09-02 22:40 +0200
        Re: [Linaro-mm-sig] [PATCHv2 3/4] staging: android: ion: Add an ioctl for ABI checking Arnd Bergmann <arnd@arndb.de> - 2016-09-02 23:40 +0200
          Re: [Linaro-mm-sig] [PATCHv2 3/4] staging: android: ion: Add an ioctl  for ABI checking Laura Abbott <labbott@redhat.com> - 2016-09-03 00:20 +0200
  [PATCHv2 2/4] staging: android: ion: Pull out ion ioctls to a separate file Laura Abbott <labbott@redhat.com> - 2016-09-02 00:50 +0200
    Re: [PATCHv2 2/4] staging: android: ion: Pull out ion ioctls to a  separate file Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-09-02 14:50 +0200
      Re: [PATCHv2 2/4] staging: android: ion: Pull out ion ioctls to a  separate file Laura Abbott <labbott@redhat.com> - 2016-09-02 22:00 +0200

csiph-web