From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753611AbcCBTuQ (ORCPT ); Wed, 2 Mar 2016 14:50:16 -0500 Received: from mail-yk0-f196.google.com ([209.85.160.196]:36699 "EHLO mail-yk0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751121AbcCBTuO (ORCPT ); Wed, 2 Mar 2016 14:50:14 -0500 Date: Wed, 2 Mar 2016 16:50:10 -0300 From: Gustavo Padovan To: Emil Velikov Cc: Greg Kroah-Hartman , devel@driverdev.osuosl.org, Daniel Stone , Daniel Vetter , Riley Andrews , ML dri-devel , "Linux-Kernel@Vger. Kernel. Org" , Arve =?iso-8859-1?B?SGr4bm5lduVn?= , Gustavo Padovan , John Harrison Subject: Re: [PATCH v5 5/5] staging/android: add flags member to sync ioctl structs Message-ID: <20160302195010.GA2446@joana> Mail-Followup-To: Gustavo Padovan , Emil Velikov , Greg Kroah-Hartman , devel@driverdev.osuosl.org, Daniel Stone , Daniel Vetter , Riley Andrews , ML dri-devel , "Linux-Kernel@Vger. Kernel. Org" , Arve =?iso-8859-1?B?SGr4bm5lduVn?= , Gustavo Padovan , John Harrison References: <1456837981-31584-1-git-send-email-gustavo@padovan.org> <1456837981-31584-5-git-send-email-gustavo@padovan.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Emil, 2016-03-02 Emil Velikov : > On 1 March 2016 at 13:13, Gustavo Padovan wrote: > > From: Gustavo Padovan > > > > Play safe and add flags member to all structs. So we don't need to > > break API or create new IOCTL in the future if new features that requires > > flags arises. > > > > v2: check if flags are valid (zero, in this case) > > > > v3: return -EINVAL if flags are not zero'ed > > > > Signed-off-by: Gustavo Padovan > > --- > > drivers/staging/android/sync.c | 8 ++++++++ > > drivers/staging/android/uapi/sync.h | 6 ++++++ > > 2 files changed, 14 insertions(+) > > > > diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c > > index 3604e453..3c265ed 100644 > > --- a/drivers/staging/android/sync.c > > +++ b/drivers/staging/android/sync.c > > @@ -445,6 +445,11 @@ static long sync_file_ioctl_merge(struct sync_file *sync_file, > > goto err_put_fd; > > } > > > > + if (data.flags) { > > + err = -EINVAL; > > + goto err_put_fd; > > + } > > + > > fence2 = sync_file_fdget(data.fd2); > > if (!fence2) { > > err = -ENOENT; > > @@ -504,6 +509,9 @@ static long sync_file_ioctl_fence_info(struct sync_file *sync_file, > > if (copy_from_user(&in, (void __user *)arg, sizeof(in))) > > return -EFAULT; > > > > + if (in.flags) > > + return -EINVAL; > > + > > info = kzalloc(sizeof(*info), GFP_KERNEL); > > if (!info) > > return -ENOMEM; > > diff --git a/drivers/staging/android/uapi/sync.h b/drivers/staging/android/uapi/sync.h > > index a122bb5..11e2d28 100644 > > --- a/drivers/staging/android/uapi/sync.h > > +++ b/drivers/staging/android/uapi/sync.h > > @@ -19,11 +19,13 @@ > > * @fd2: file descriptor of second fence > > * @name: name of new fence > > * @fence: returns the fd of the new fence to userspace > > + * @flags: merge_data flags > > */ > > struct sync_merge_data { > > __s32 fd2; > > char name[32]; > > __s32 fence; > > + __u32 flags; > The comment from last round still stands, struct size must be multiple > of 64bits. As is the struct will be broken whenever/if we decide to > extend it. See [1] for an alternative wording. > > > }; > > > > /** > > @@ -31,12 +33,14 @@ struct sync_merge_data { > > * @obj_name: name of parent sync_timeline > > * @driver_name: name of driver implementing the parent > > * @status: status of the fence 0:active 1:signaled <0:error > > + * @flags: fence_info flags > > * @timestamp_ns: timestamp of status change in nanoseconds > > */ > > struct sync_fence_info { > > char obj_name[32]; > > char driver_name[32]; > > __s32 status; > > + __u32 flags; > > __u64 timestamp_ns; > > }; > > > > @@ -44,6 +48,7 @@ struct sync_fence_info { > > * struct sync_file_info - data returned from fence info ioctl > > * @name: name of fence > > * @status: status of fence. 1: signaled 0:active <0:error > > + * @flags: sync_file_info flags > > * @num_fences number of fences in the sync_file > > * @sync_fence_info: pointer to array of structs sync_fence_info with all > > * fences in the sync_file > > @@ -51,6 +56,7 @@ struct sync_fence_info { > > struct sync_file_info { > > char name[32]; > > __s32 status; > > + __u32 flags; > > __u32 num_fences; > > > > __u64 sync_fence_info; > Thanks for taking my suggestion and dropping len. Although I fear that > we introduced a hole which we should be explicitly padded [2]. > > In both cases the pad should be checked for 0 and -EINVAL should be > returned if that's not the case. This will allow us to potentially > reuse in the future. > > Other than that I believe the series looks pretty much spot on. I agree with both suggestions, a new version of the patches is on the way. Gustavo