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

Groups > linux.kernel > #1475438

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

Show all headers | View raw

On 09/02/2016 02:02 AM, Arnd Bergmann wrote:
> 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).
>

This was just a structure inside ion_ioctl. I pulled it out for
the validate function. It's not an actual argument to any ioctl from
userspace. ion_ioctl copies using _IOC_SIZE.

>> +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.
>

The concern was trying ioctls that wouldn't actually fail or would
have some other unexpected side effect.

My conclusion from the other thread was that assuming we don't botch
up adding new ioctls in the future or make incompatible changes to
these in the future we shouldn't technically need it. I was still
trying to hedge my bets against the future but that might just be
making the problem worse?

>> +/**
>> + * 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.
>

The botching-ioctls.txt document suggested everything should be aligned
to 64-bits. Was I interpreting that too literally?

> 	Arnd
>

Thanks,
Laura

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