From patchwork Tue Mar 1 13:13:00 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Gustavo Padovan X-Patchwork-Id: 8465091 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id A42CB9F38C for ; Tue, 1 Mar 2016 13:13:25 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id A6700202AE for ; Tue, 1 Mar 2016 13:13:24 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id B43922013D for ; Tue, 1 Mar 2016 13:13:23 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id E6A766E5F6; Tue, 1 Mar 2016 13:13:21 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-yw0-f196.google.com (mail-yw0-f196.google.com [209.85.161.196]) by gabe.freedesktop.org (Postfix) with ESMTPS id 85B9E6E5F5 for ; Tue, 1 Mar 2016 13:13:20 +0000 (UTC) Received: by mail-yw0-f196.google.com with SMTP id h80so9294537ywc.3 for ; Tue, 01 Mar 2016 05:13:20 -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=qHTJmYOA+CCyr1R1BDnSogeLF/hHchZuXDIlTQd9mc0=; b=LBU61IQ5GAvV67PFn9YF+KWHractvLXnpdWaWzrpShoo3ZTBrmxCP+WbpiZJzloAEp qd6x61v4Cj4xtenGT5VgF8Dyn8w42bVVz5HBKFqSR4npzVkkCdoTxUgKEKy6TtL1ORor kb8lwR7eSOIYBjxQZkomxBqaFOZvcDJT7oFumjsjVVRKZVzBgdVQT8flLB3uwuia2mIy zZXoDMhgql/b+3F3oa1WsXIdWTnI/IvlFldDIAYj5CaKwPW996U0/9q4vrTyg0gaAQGZ Tmm0I5Qsi6Lxm/rZQ2y700YwrtOntL9JopWTKKzJ7z9zq330tRgQi6A0FHZGNNm8HeiH 6mjw== X-Gm-Message-State: AD7BkJLbc82NlRPR4di86hsxfR9CnQ25pAHqamLy/aBOixex5d7ydINJMTZTIuDZ5mxoEQ== X-Received: by 10.129.76.17 with SMTP id z17mr11049793ywa.191.1456837999482; Tue, 01 Mar 2016 05:13:19 -0800 (PST) Received: from jade.localdomain ([177.194.47.65]) by smtp.gmail.com with ESMTPSA id d8sm23442780ywc.26.2016.03.01.05.13.16 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 01 Mar 2016 05:13:18 -0800 (PST) From: Gustavo Padovan To: Greg Kroah-Hartman Subject: [PATCH v5 4/5] staging/android: refactor SYNC_IOC_FILE_INFO Date: Tue, 1 Mar 2016 10:13:00 -0300 Message-Id: <1456837981-31584-4-git-send-email-gustavo@padovan.org> X-Mailer: git-send-email 2.5.0 In-Reply-To: <1456837981-31584-1-git-send-email-gustavo@padovan.org> References: <1456837981-31584-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=-2.9 required=5.0 tests=BAYES_00, RCVD_IN_BL_SPAMCOP_NET, 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); } 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 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 '>'