Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753541AbcD2OQl (ORCPT ); Fri, 29 Apr 2016 10:16:41 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:50584 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752764AbcD2OQk (ORCPT ); Fri, 29 Apr 2016 10:16:40 -0400 Date: Fri, 29 Apr 2016 11:16:25 -0300 From: Gustavo Padovan To: Hillf Danton Cc: linux-kernel , Daniel Vetter Subject: Re: [PATCH v2 06/13] staging/android: remove name arg from sync_file_create() Message-ID: <20160429141625.GE2524@joana> References: <010301d1a1f5$14bb8c70$3e32a550$@alibaba-inc.com> <010a01d1a1f5$c9a9c160$5cfd4420$@alibaba-inc.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <010a01d1a1f5$c9a9c160$5cfd4420$@alibaba-inc.com> 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 Content-Length: 4757 Lines: 129 Hi Hillf, (please keep the whole cc on the reply, so others can see it too) 2016-04-29 Hillf Danton : > > > > From: Gustavo Padovan > > > > Simplifies the API to only receive the fence it needs to add to the > > sync and create a name for the sync_file based on the fence context and > > seqno. > > > > Signed-off-by: Gustavo Padovan > > Reviewed-by: Daniel Vetter > > --- > > drivers/staging/android/sync.c | 16 +++++++++------- > > drivers/staging/android/sync.h | 2 +- > > drivers/staging/android/sync_debug.c | 3 +-- > > 3 files changed, 11 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c > > index 7e0fa20..5470ae9 100644 > > --- a/drivers/staging/android/sync.c > > +++ b/drivers/staging/android/sync.c > > @@ -136,7 +136,7 @@ struct fence *sync_pt_create(struct sync_timeline *obj, int size) > > } > > EXPORT_SYMBOL(sync_pt_create); > > > > -static struct sync_file *sync_file_alloc(int size, const char *name) > > +static struct sync_file *sync_file_alloc(int size) > > { > > struct sync_file *sync_file; > > > > @@ -150,7 +150,6 @@ static struct sync_file *sync_file_alloc(int size, const char *name) > > goto err; > > > > kref_init(&sync_file->kref); > > - strlcpy(sync_file->name, name, sizeof(sync_file->name)); > > > > init_waitqueue_head(&sync_file->wq); > > > > @@ -175,23 +174,25 @@ static void fence_check_cb_func(struct fence *f, struct fence_cb *cb) > > > > /** > > * sync_fence_create() - creates a sync fence > > - * @name: name of fence to create > > * @fence: fence to add to the sync_fence > > * > > * Creates a sync_file containg @fence. Once this is called, the sync_file > > * takes ownership of @fence. > > */ > > -struct sync_file *sync_file_create(const char *name, struct fence *fence) > > +struct sync_file *sync_file_create(struct fence *fence) > > { > > struct sync_file *sync_file; > > > > - sync_file = sync_file_alloc(offsetof(struct sync_file, cbs[1]), > > - name); > > + sync_file = sync_file_alloc(offsetof(struct sync_file, cbs[1])); > > if (!sync_file) > > return NULL; > > > > sync_file->num_fences = 1; > > atomic_set(&sync_file->status, 1); > > + snprintf(sync_file->name, sizeof(sync_file->name), "%s-%s%d-%d", > > + fence->ops->get_driver_name(fence), > > + fence->ops->get_timeline_name(fence), fence->context, > > + fence->seqno); > > > > sync_file->cbs[0].fence = fence; > > sync_file->cbs[0].sync_file = sync_file; > > @@ -260,7 +261,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, > > int i, i_a, i_b; > > unsigned long size = offsetof(struct sync_file, cbs[num_fences]); > > > > - sync_file = sync_file_alloc(size, name); > > + sync_file = sync_file_alloc(size); > > if (!sync_file) > > return NULL; > > > > @@ -306,6 +307,7 @@ static struct sync_file *sync_file_merge(const char *name, struct sync_file *a, > > atomic_sub(num_fences - i, &sync_file->status); > > sync_file->num_fences = i; > > > > + strlcpy(sync_file->name, name, sizeof(sync_file->name)); > > sync_file_debug_add(sync_file); > > return sync_file; > > } > > diff --git a/drivers/staging/android/sync.h b/drivers/staging/android/sync.h > > index 1f164df..7dee444 100644 > > --- a/drivers/staging/android/sync.h > > +++ b/drivers/staging/android/sync.h > > @@ -167,7 +167,7 @@ void sync_timeline_signal(struct sync_timeline *obj); > > */ > > struct fence *sync_pt_create(struct sync_timeline *parent, int size); > > > > -struct sync_file *sync_file_create(const char *name, struct fence *fence); > > +struct sync_file *sync_file_create(struct fence *fence); > > > > #ifdef CONFIG_DEBUG_FS > > > > diff --git a/drivers/staging/android/sync_debug.c b/drivers/staging/android/sync_debug.c > > index e4b0e41..2cab40d 100644 > > --- a/drivers/staging/android/sync_debug.c > > +++ b/drivers/staging/android/sync_debug.c > > @@ -262,8 +262,7 @@ static long sw_sync_ioctl_create_fence(struct sw_sync_timeline *obj, > > goto err; > > } > > > > - data.name[sizeof(data.name) - 1] = '\0'; > > - sync_file = sync_file_create(data.name, fence); > > + sync_file = sync_file_create(fence); > > > The name injected from user spce is ignored, why? > Is it a semantic change? It is. sw_sync is only a testing API that we are not de-staging now. I think we we will just remove the name arg from the ioctl there. SW_SYNC ioctl is only for debugging test, we don't want to publicly export its ABI in the kernel uapi headers. > Mentioned in commit message? Sure, I can update the commit message. Gustavo