Re: [PATCH v2 06/13] staging/android: remove name arg from sync_file_create()

Re: [PATCH v2 06/13] staging/android: remove name arg from sync_file_create()

[Hillf Danton]

> 
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> 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 <gustavo.padovan@collabora.co.uk>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  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?
Mentioned in commit message?

>  	if (!sync_file) {
>  		fence_put(fence);
>  		err = -ENOMEM;
> --
> 2.5.5
> 
> 

Re: [PATCH v2 06/13] staging/android: remove name arg from sync_file_create()

[Gustavo Padovan]

Hi Hillf,

(please keep the whole cc on the reply, so others can see it too)

2016-04-29 Hillf Danton <hillf.zj@alibaba-inc.com>:

> > 
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > 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 <gustavo.padovan@collabora.co.uk>
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  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

Re: [PATCH v2 06/13] staging/android: remove name arg from sync_file_create()

[Hillf Danton]

> 
> 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.
> 
JFYI, be mindful please of removing the arg. If current uses are
broken, the mainline maintainer, MM, can be stung another time.

Hillf

[PATCH v2 00/13] De-stage Sync File Framework

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

Hi,

This patchset sits on top of Sync ABI Rework v13:

https://www.spinics.net/lists/dri-devel/msg105667.html

The first eight clean up and prepare sync_file for de-staging. The last four
patches do the de-staging, moving files to drivers/dma-buf/ and include/linux/
plus adding Documentation.

As the de-stage depends upon many changes on the staging tree it would
be good to get all the patches merged through the staging tree if Sumit
agrees with that.

The next step on the Sync de-stage is clean up the remaining bits
of the Sync Framework, mainly SW_SYNC, which is only used for testing.

v2: - Add Reviewed-by: tag from Daniel Vetter to all patches.
    - Take in sugestions for the Sync File Documentation (Daniel)
    - Remove name arg from sync_file_crate() (Daniel)
    - Revome leftover EXPORT_SYMBOL(sync_file_merge) (Daniel)

Thanks,

	Gustavo

Gustavo Padovan (13):
  staging/android: remove redundant comments on sync_merge_data
  staging/android: drop sync_file_install() and sync_file_put()
  staging/android: move sync_file functions comments to sync.c
  staging/android: make sync_file_merge() static
  staging/android: make sync_file_fdget() static
  staging/android: remove name arg from sync_file_create()
  staging/android: prepare sync_file for de-staging
  staging/android: improve documentation for sync_file
  staging/android: style fix: alignment to match the open parenthesis
  dma-buf/sync_file: de-stage sync_file headers
  dma-buf/sync_file: de-stage sync_file
  Documentation: include sync_file into DocBook
  Documentation: add Sync File doc

 Documentation/DocBook/device-drivers.tmpl |   2 +
 Documentation/sync_file.txt               |  69 ++++++
 drivers/Kconfig                           |   2 +
 drivers/dma-buf/Kconfig                   |  11 +
 drivers/dma-buf/Makefile                  |   1 +
 drivers/dma-buf/sync_file.c               | 395 ++++++++++++++++++++++++++++++
 drivers/staging/android/Kconfig           |   1 +
 drivers/staging/android/sync.c            | 362 ---------------------------
 drivers/staging/android/sync.h            |  91 +------
 drivers/staging/android/sync_debug.c      |   8 +-
 drivers/staging/android/uapi/sync.h       | 100 --------
 include/linux/sync_file.h                 |  57 +++++
 include/uapi/linux/sync_file.h            | 100 ++++++++
 13 files changed, 644 insertions(+), 555 deletions(-)
 create mode 100644 Documentation/sync_file.txt
 create mode 100644 drivers/dma-buf/Kconfig
 create mode 100644 drivers/dma-buf/sync_file.c
 delete mode 100644 drivers/staging/android/uapi/sync.h
 create mode 100644 include/linux/sync_file.h
 create mode 100644 include/uapi/linux/sync_file.h

-- 
2.5.5

[PATCH v2 06/13] staging/android: remove name arg from sync_file_create()

[Gustavo Padovan]

From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>

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 <gustavo.padovan@collabora.co.uk>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 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);
 	if (!sync_file) {
 		fence_put(fence);
 		err = -ENOMEM;
-- 
2.5.5