From patchwork Thu Mar 3 19:40:45 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Gustavo Padovan X-Patchwork-Id: 8495801 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 39D89C0553 for ; Thu, 3 Mar 2016 19:41:14 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 34BBF2038E for ; Thu, 3 Mar 2016 19:41:13 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 2DE932037C for ; Thu, 3 Mar 2016 19:41:12 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id DF92E6EAFA; Thu, 3 Mar 2016 19:41:10 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-yk0-f175.google.com (mail-yk0-f175.google.com [209.85.160.175]) by gabe.freedesktop.org (Postfix) with ESMTPS id 64D9F6EAFD for ; Thu, 3 Mar 2016 19:41:08 +0000 (UTC) Received: by mail-yk0-f175.google.com with SMTP id z13so13814566ykd.0 for ; Thu, 03 Mar 2016 11:41:08 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=qbbtot8zYNnGnXuEYLw1pwukfGYNaoARlSDn4wa07JQ=; b=bjQodvasfxZG1xMUh5qh4XkWsejlmu0v2lWiXEVaBdk9PbDnNlJCqCly/+fRzT/3zG UE3jYXiLvHhSywaY82l6iMFESAMb3a3Xe/L1FS8HHu3BuTJdCXwpg0FC2+HtxR6Pgd3g H/9qeKv80L6MEqPDMmro1l1BPH3nV2AG+Xr97DSYKlmxqtllTlgD0J1arVfkHiHRxwCI EBHuFcsax6Ud5rca+z6ZkRg16Wky6/1iC5GkxpTpozM2vZu4KHLMjIePkgqhJveYKTTa +UM/GVW9LZ/2Gm8+HtKvA2sayPbsDWuI5+1XRMF4F3wgRU+kjP8evONdJyNZg1qzFQ4w NRaQ== X-Gm-Message-State: AD7BkJJ1NB0FW2QmuXVxDG/nL0q4AproZSwLPU4RA9NUSNc/NyXKfdkAXTO3mZw7aumfpA== X-Received: by 10.37.57.202 with SMTP id g193mr2366943yba.41.1457034067243; Thu, 03 Mar 2016 11:41:07 -0800 (PST) Received: from jade.localdomain ([177.194.47.65]) by smtp.gmail.com with ESMTPSA id f186sm28850ywa.6.2016.03.03.11.41.03 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 03 Mar 2016 11:41:06 -0800 (PST) From: Gustavo Padovan To: Greg Kroah-Hartman Subject: [PATCH v7 4/5] staging/android: refactor SYNC_IOC_FILE_INFO Date: Thu, 3 Mar 2016 16:40:45 -0300 Message-Id: <1457034046-27678-4-git-send-email-gustavo@padovan.org> X-Mailer: git-send-email 2.5.0 In-Reply-To: <1457034046-27678-1-git-send-email-gustavo@padovan.org> References: <1457034046-27678-1-git-send-email-gustavo@padovan.org> Cc: devel@driverdev.osuosl.org, Daniel Stone , Daniel Vetter , Riley Andrews , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, =?UTF-8?q?Arve=20Hj=C3=B8nnev=C3=A5g?= , Gustavo Padovan , John Harrison X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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); } Signed-off-by: Gustavo Padovan Reviewed-by: Maarten Lankhorst --- 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 v4: remove allocated struct sync_file_info (comment from Maarten) --- drivers/staging/android/sync.c | 70 +++++++++++++++++-------------------- drivers/staging/android/uapi/sync.h | 9 ++--- 2 files changed, 36 insertions(+), 43 deletions(-) diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c index dc5f382..48ee175 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,60 +491,60 @@ 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 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(&info, (void __user *)arg, sizeof(info))) return -EFAULT; - if (size < sizeof(struct sync_file_info)) - return -EINVAL; + /* + * 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 (!info.num_fences) + goto no_fences; - if (size > 4096) - size = 4096; + if (info.num_fences < sync_file->num_fences) + return -EINVAL; - info = kzalloc(size, GFP_KERNEL); - if (!info) + size = sync_file->num_fences * sizeof(*fence_info); + fence_info = kzalloc(size, GFP_KERNEL); + if (!fence_info) return -ENOMEM; - strlcpy(info->name, sync_file->name, sizeof(info->name)); - info->status = atomic_read(&sync_file->status); - if (info->status >= 0) - info->status = !info->status; + for (i = 0; i < sync_file->num_fences; ++i) + sync_fill_fence_info(sync_file->cbs[i].fence, &fence_info[i]); - info->num_fences = sync_file->num_fences; - - len = sizeof(struct sync_file_info); - - for (i = 0; i < sync_file->num_fences; ++i) { - struct fence *fence = sync_file->cbs[i].fence; - - ret = sync_fill_fence_info(fence, (u8 *)info + len, size - len); - - if (ret < 0) - goto out; - - len += ret; + if (copy_to_user((void __user *)info.sync_fence_info, fence_info, + size)) { + ret = -EFAULT; + goto out; } - info->len = len; +no_fences: + strlcpy(info.name, sync_file->name, sizeof(info.name)); + info.status = atomic_read(&sync_file->status); + if (info.status >= 0) + info.status = !info.status; + + 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(info); + kfree(fence_info); return ret; } diff --git a/drivers/staging/android/uapi/sync.h b/drivers/staging/android/uapi/sync.h index f0b41ce..a122bb5 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 '>'