From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752400AbcIBJCf (ORCPT ); Fri, 2 Sep 2016 05:02:35 -0400 Received: from mout.kundenserver.de ([212.227.126.187]:51947 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751716AbcIBJCd (ORCPT ); Fri, 2 Sep 2016 05:02:33 -0400 From: Arnd Bergmann To: linaro-mm-sig@lists.linaro.org Cc: Laura Abbott , Sumit Semwal , John Stultz , Arve =?ISO-8859-1?Q?Hj=F8nnev=E5g?= , Riley Andrews , devel@driverdev.osuosl.org, Jon Medhurst , Android Kernel Team , Liviu Dudau , linux-kernel@vger.kernel.org, Jeremy Gebben , Eun Taik Lee , Greg Kroah-Hartman , Brian Starkey , Chen Feng Subject: Re: [Linaro-mm-sig] [PATCHv2 3/4] staging: android: ion: Add an ioctl for ABI checking Date: Fri, 02 Sep 2016 11:02:13 +0200 Message-ID: <7145471.hhkX02hEGi@wuerfel> User-Agent: KMail/5.1.3 (Linux/4.4.0-34-generic; KDE/5.18.0; x86_64; ; ) In-Reply-To: <1472769644-11039-4-git-send-email-labbott@redhat.com> References: <1472769644-11039-1-git-send-email-labbott@redhat.com> <1472769644-11039-4-git-send-email-labbott@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:iFlr5meuKKiL1odsh/ajaCwvUuL7xdLBGVy4vI4H9XeAq4IDOOU 1n7njbUATg0RW/Pxr9wZntB2Baizs26FVXOMp0ZJsm6ij/ziPDNm/FGdGP4Xsrl4nkzAC73 9JbvYRBfA4ItUBvDvVGkE4PY0fhXsk7RKyhnsVPoW5eS0Nn7VYvNyjSEl/0b2a+CtphM5Zo jcHs4YIbfuXoX1L/83axw== X-UI-Out-Filterresults: notjunk:1;V01:K0:nFuugE8DzMg=:Uux5K9wgB2cmyvEqOey0nE x/RU1B/DAMxmKdiQhGpBywikMiSzLW0lE9cT7uTltmQ+hu5HueEjyXnLmpqzmWDLRC83YJTvW RXxv5quR2ccI/hhFyp3rKv93kizDTCbHYWCP+uH35bQvp3PEFCA9nopX3vPzy/Rlw6gaa+2jH rCjHgwaZPXUePdG9CHPIsfIBI6AvA7MxL0OR7SPi+mYWo1Vi9ugH8KXvKxF9oaUyMMIpgJKVV MNCeLh55kTQ7VWB+MQdBSTbs9ZRdKyYfE2l70wOpdcsHmEHNCvKYMmPw9KylCrNHs0WoCF+B9 MrrhcAFxkFFf6Gk+3/ajv23vAZ/+zkxgeo8fpLA0yayhlBAgXHfOebEGYj777Yn2nGZBg9oV/ JsjQ8FROzc5DLywUc88r/MK9q8AoKOWsDDkv4gYFBwzu/Lwa5fv5YmI0z/zOoRLT6smUH6sCE VpLsfv1A+UFYaeboLK3VK6HJHpjeouLkh4d7aL5VBt5YBgZsePpxUlirsaCO569/SOuldHVgQ J1+b7R0JqTvY4QU1Gz1hpzwps04Osv/rvzW3DT91A/qPufAkrZfVsl9jLzrrEklpntC3aXHX9 iKsUbPJNGi0sZPQ7g386rguPRRsh6gBxJKASxxJhF1Djy3/Akvt04ebQtU7m/ZIpqc90hSXhO 4dzXK8XptKm82YinkhFIln5DB+1cXPFsn58eJ/65zbzszEPaNGJN+8bxjnmbRDR4I0eLhxILe H7vY0xTGVK5K9efz Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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