Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755392AbcCBTwi (ORCPT ); Wed, 2 Mar 2016 14:52:38 -0500 Received: from mail-yw0-f194.google.com ([209.85.161.194]:32982 "EHLO mail-yw0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753062AbcCBTwf (ORCPT ); Wed, 2 Mar 2016 14:52:35 -0500 From: Gustavo Padovan To: Greg Kroah-Hartman Cc: linux-kernel@vger.kernel.org, devel@driverdev.osuosl.org, dri-devel@lists.freedesktop.org, Daniel Stone , =?UTF-8?q?Arve=20Hj=C3=B8nnev=C3=A5g?= , Riley Andrews , Daniel Vetter , Rob Clark , Greg Hackmann , John Harrison , Maarten Lankhorst , Gustavo Padovan Subject: [PATCH v6 5/6] staging/android: refactor SYNC_IOC_FILE_INFO Date: Wed, 2 Mar 2016 16:52:10 -0300 Message-Id: <1456948331-7300-5-git-send-email-gustavo@padovan.org> X-Mailer: git-send-email 2.5.0 In-Reply-To: <1456948331-7300-1-git-send-email-gustavo@padovan.org> References: <1456948331-7300-1-git-send-email-gustavo@padovan.org> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6111 Lines: 209 From: Gustavo Padovan Change SYNC_IOC_FILE_INFO behaviour to avoid future API breaks and optimize buffer Now num_fences can be filled by the caller to inform how many fences it wants to retrieve from the kernel. If the num_fences passed is greater than zero info->sync_fence_info should point to a buffer with enough space to fit all fences. However if num_fences passed to the kernel is 0, the kernel will reply with number of fences of the sync_file. Sending first an ioctl with num_fences = 0 can optimize buffer allocation, in a first call with num_fences = 0 userspace will receive the actual number of fences in the num_fences filed. Then it can allocate a buffer with the correct size on sync_fence_info and call SYNC_IOC_FILE_INFO again, but now with the actual value of num_fences in the sync_file. Also, info->sync_fence_info was converted to __u64 pointer to prevent 32bit compatibility issues. An example userspace code for the later would be: struct sync_file_info *info; int err, size, num_fences; info = malloc(sizeof(*info)); info.flags = 0; err = ioctl(fd, SYNC_IOC_FILE_INFO, info); num_fences = info->num_fences; if (num_fences) { info.flags = 0; size = sizeof(struct sync_fence_info) * num_fences; info->num_fences = num_fences; info->sync_fence_info = (uint64_t) calloc(num_fences, sizeof(struct sync_fence_info)); err = ioctl(fd, SYNC_IOC_FILE_INFO, info); } v2: fix fence_info memory leak v3: Comments from Emil Velikov - improve commit message - remove __u64 cast - remove check for output fields in file_info - clean up sync_fill_fence_info() Comments from Maarten Lankhorst - remove in.num_fences && !in.sync_fence_info check - remove info->len and use only num_fences to calculate size Comments from Dan Carpenter - fix info->sync_fence_info documentation Signed-off-by: Gustavo Padovan --- drivers/staging/android/sync.c | 64 ++++++++++++++++++++----------------- drivers/staging/android/uapi/sync.h | 9 ++---- 2 files changed, 38 insertions(+), 35 deletions(-) diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c index dc5f382..3604e453 100644 --- a/drivers/staging/android/sync.c +++ b/drivers/staging/android/sync.c @@ -479,13 +479,9 @@ err_put_fd: return err; } -static int sync_fill_fence_info(struct fence *fence, void *data, int size) +static void sync_fill_fence_info(struct fence *fence, + struct sync_fence_info *info) { - struct sync_fence_info *info = data; - - if (size < sizeof(*info)) - return -ENOMEM; - strlcpy(info->obj_name, fence->ops->get_timeline_name(fence), sizeof(info->obj_name)); strlcpy(info->driver_name, fence->ops->get_driver_name(fence), @@ -495,28 +491,20 @@ static int sync_fill_fence_info(struct fence *fence, void *data, int size) else info->status = 0; info->timestamp_ns = ktime_to_ns(fence->timestamp); - - return sizeof(*info); } static long sync_file_ioctl_fence_info(struct sync_file *sync_file, unsigned long arg) { - struct sync_file_info *info; + struct sync_file_info in, *info; + struct sync_fence_info *fence_info = NULL; __u32 size; - __u32 len = 0; int ret, i; - if (copy_from_user(&size, (void __user *)arg, sizeof(size))) + if (copy_from_user(&in, (void __user *)arg, sizeof(in))) return -EFAULT; - if (size < sizeof(struct sync_file_info)) - return -EINVAL; - - if (size > 4096) - size = 4096; - - info = kzalloc(size, GFP_KERNEL); + info = kzalloc(sizeof(*info), GFP_KERNEL); if (!info) return -ENOMEM; @@ -525,29 +513,47 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file, if (info->status >= 0) info->status = !info->status; - info->num_fences = sync_file->num_fences; - - len = sizeof(struct sync_file_info); + /* + * Passing num_fences = 0 means that userspace doesn't want to + * retrieve any sync_fence_info. If num_fences = 0 we skip filling + * sync_fence_info and return the actual number of fences on + * info->num_fences. + */ + if (!in.num_fences) + goto no_fences; - for (i = 0; i < sync_file->num_fences; ++i) { - struct fence *fence = sync_file->cbs[i].fence; + if (in.num_fences < sync_file->num_fences) { + ret = -EINVAL; + goto out; + } - ret = sync_fill_fence_info(fence, (u8 *)info + len, size - len); + size = sync_file->num_fences * sizeof(*fence_info); + fence_info = kzalloc(size, GFP_KERNEL); + if (!fence_info) { + ret = -ENOMEM; + goto out; + } - if (ret < 0) - goto out; + for (i = 0; i < sync_file->num_fences; ++i) + sync_fill_fence_info(sync_file->cbs[i].fence, &fence_info[i]); - len += ret; + if (copy_to_user((void __user *)in.sync_fence_info, fence_info, size)) { + ret = -EFAULT; + goto out; } - info->len = len; + info->sync_fence_info = in.sync_fence_info; + +no_fences: + info->num_fences = sync_file->num_fences; - if (copy_to_user((void __user *)arg, info, len)) + if (copy_to_user((void __user *)arg, info, sizeof(*info))) ret = -EFAULT; else ret = 0; out: + kfree(fence_info); kfree(info); return ret; diff --git a/drivers/staging/android/uapi/sync.h b/drivers/staging/android/uapi/sync.h index a6c648c..f064923 100644 --- a/drivers/staging/android/uapi/sync.h +++ b/drivers/staging/android/uapi/sync.h @@ -42,21 +42,18 @@ struct sync_fence_info { /** * struct sync_file_info - data returned from fence info ioctl - * @len: ioctl caller writes the size of the buffer its passing in. - * ioctl returns length of sync_file_info returned to - * userspace including pt_info. * @name: name of fence * @status: status of fence. 1: signaled 0:active <0:error * @num_fences number of fences in the sync_file - * @sync_fence_info: array of sync_fence_info for every fence in the sync_file + * @sync_fence_info: pointer to array of structs sync_fence_info with all + * fences in the sync_file */ struct sync_file_info { - __u32 len; char name[32]; __s32 status; __u32 num_fences; - __u8 sync_fence_info[0]; + __u64 sync_fence_info; }; #define SYNC_IOC_MAGIC '>' -- 2.5.0