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