Re: [PATCH v2 02/13] staging/android: drop sync_file_install() and sync_file_put()

Re: [PATCH v2 02/13] staging/android: drop sync_file_install() and sync_file_put()

[Hillf Danton]

> 
> From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> 
> These two functions are just wrappers for one line functions, they
> call fd_install() and fput() respectively, so just get rid of them
> and use fd_install() and fput() directly for more simplicity.
> 
Given sync_file is not file, I don't see that simplicity is worth of
the change of 20+ lines.
Can you please specify the disadvantages of the wrappers?

> Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/staging/android/sync.c       | 20 ++++----------------
>  drivers/staging/android/sync.h       | 19 -------------------
>  drivers/staging/android/sync_debug.c |  4 ++--
>  3 files changed, 6 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c
> index f9c6094..b965e2a 100644
> --- a/drivers/staging/android/sync.c
> +++ b/drivers/staging/android/sync.c
> @@ -216,18 +216,6 @@ err:
>  }
>  EXPORT_SYMBOL(sync_file_fdget);
> 
> -void sync_file_put(struct sync_file *sync_file)
> -{
> -	fput(sync_file->file);
> -}
> -EXPORT_SYMBOL(sync_file_put);
> -
> -void sync_file_install(struct sync_file *sync_file, int fd)
> -{
> -	fd_install(fd, sync_file->file);
> -}
> -EXPORT_SYMBOL(sync_file_install);
> -
>  static void sync_file_add_pt(struct sync_file *sync_file, int *i,
>  			     struct fence *fence)
>  {
> @@ -469,15 +457,15 @@ static long sync_file_ioctl_merge(struct sync_file *sync_file,
>  		goto err_put_fence3;
>  	}
> 
> -	sync_file_install(fence3, fd);
> -	sync_file_put(fence2);
> +	fd_install(fd, fence3->file);
> +	fput(fence2->file);
>  	return 0;
> 
>  err_put_fence3:
> -	sync_file_put(fence3);
> +	fput(fence3->file);
> 
>  err_put_fence2:
> -	sync_file_put(fence2);
> +	fput(fence2->file);
> 
>  err_put_fd:
>  	put_unused_fd(fd);
> diff --git a/drivers/staging/android/sync.h b/drivers/staging/android/sync.h
> index d2a1734..c45cc7b 100644
> --- a/drivers/staging/android/sync.h
> +++ b/drivers/staging/android/sync.h
> @@ -203,25 +203,6 @@ struct sync_file *sync_file_merge(const char *name,
>   */
>  struct sync_file *sync_file_fdget(int fd);
> 
> -/**
> - * sync_file_put() - puts a reference of a sync_file
> - * @sync_file:	sync_file to put
> - *
> - * Puts a reference on @sync_fence.  If this is the last reference, the
> - * sync_fil and all it's sync_pts will be freed
> - */
> -void sync_file_put(struct sync_file *sync_file);
> -
> -/**
> - * sync_file_install() - installs a sync_file into a file descriptor
> - * @sync_file:	sync_file to install
> - * @fd:		file descriptor in which to install the fence
> - *
> - * Installs @sync_file into @fd.  @fd's should be acquired through
> - * get_unused_fd_flags(O_CLOEXEC).
> - */
> -void sync_file_install(struct sync_file *sync_file, int fd);
> -
>  #ifdef CONFIG_DEBUG_FS
> 
>  void sync_timeline_debug_add(struct sync_timeline *obj);
> diff --git a/drivers/staging/android/sync_debug.c b/drivers/staging/android/sync_debug.c
> index 5a7ec58..e4b0e41 100644
> --- a/drivers/staging/android/sync_debug.c
> +++ b/drivers/staging/android/sync_debug.c
> @@ -272,12 +272,12 @@ static long sw_sync_ioctl_create_fence(struct sw_sync_timeline *obj,
> 
>  	data.fence = fd;
>  	if (copy_to_user((void __user *)arg, &data, sizeof(data))) {
> -		sync_file_put(sync_file);
> +		fput(sync_file->file);
>  		err = -EFAULT;
>  		goto err;
>  	}
> 
> -	sync_file_install(sync_file, fd);
> +	fd_install(fd, sync_file->file);
> 
>  	return 0;
> 
> --
> 2.5.5
> 
> 

Re: [PATCH v2 02/13] staging/android: drop sync_file_install() and sync_file_put()

[Gustavo Padovan]

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

> > 
> > From: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
> > 
> > These two functions are just wrappers for one line functions, they
> > call fd_install() and fput() respectively, so just get rid of them
> > and use fd_install() and fput() directly for more simplicity.
> > 
> Given sync_file is not file, I don't see that simplicity is worth of
> the change of 20+ lines.
> Can you please specify the disadvantages of the wrappers?

Our idea here was to simplify as much as possible this API, also we do
not want to hide sync_file internals from its users. This is staging
and we are re-thinking the API (and userspace ABI too) before moving it
out if staging. I can add some more information to the commit message.

	Gustavo

[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 02/13] staging/android: drop sync_file_install() and sync_file_put()

[Gustavo Padovan]

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

These two functions are just wrappers for one line functions, they
call fd_install() and fput() respectively, so just get rid of them
and use fd_install() and fput() directly for more simplicity.

Signed-off-by: Gustavo Padovan <gustavo.padovan@collabora.co.uk>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/staging/android/sync.c       | 20 ++++----------------
 drivers/staging/android/sync.h       | 19 -------------------
 drivers/staging/android/sync_debug.c |  4 ++--
 3 files changed, 6 insertions(+), 37 deletions(-)

diff --git a/drivers/staging/android/sync.c b/drivers/staging/android/sync.c
index f9c6094..b965e2a 100644
--- a/drivers/staging/android/sync.c
+++ b/drivers/staging/android/sync.c
@@ -216,18 +216,6 @@ err:
 }
 EXPORT_SYMBOL(sync_file_fdget);
 
-void sync_file_put(struct sync_file *sync_file)
-{
-	fput(sync_file->file);
-}
-EXPORT_SYMBOL(sync_file_put);
-
-void sync_file_install(struct sync_file *sync_file, int fd)
-{
-	fd_install(fd, sync_file->file);
-}
-EXPORT_SYMBOL(sync_file_install);
-
 static void sync_file_add_pt(struct sync_file *sync_file, int *i,
 			     struct fence *fence)
 {
@@ -469,15 +457,15 @@ static long sync_file_ioctl_merge(struct sync_file *sync_file,
 		goto err_put_fence3;
 	}
 
-	sync_file_install(fence3, fd);
-	sync_file_put(fence2);
+	fd_install(fd, fence3->file);
+	fput(fence2->file);
 	return 0;
 
 err_put_fence3:
-	sync_file_put(fence3);
+	fput(fence3->file);
 
 err_put_fence2:
-	sync_file_put(fence2);
+	fput(fence2->file);
 
 err_put_fd:
 	put_unused_fd(fd);
diff --git a/drivers/staging/android/sync.h b/drivers/staging/android/sync.h
index d2a1734..c45cc7b 100644
--- a/drivers/staging/android/sync.h
+++ b/drivers/staging/android/sync.h
@@ -203,25 +203,6 @@ struct sync_file *sync_file_merge(const char *name,
  */
 struct sync_file *sync_file_fdget(int fd);
 
-/**
- * sync_file_put() - puts a reference of a sync_file
- * @sync_file:	sync_file to put
- *
- * Puts a reference on @sync_fence.  If this is the last reference, the
- * sync_fil and all it's sync_pts will be freed
- */
-void sync_file_put(struct sync_file *sync_file);
-
-/**
- * sync_file_install() - installs a sync_file into a file descriptor
- * @sync_file:	sync_file to install
- * @fd:		file descriptor in which to install the fence
- *
- * Installs @sync_file into @fd.  @fd's should be acquired through
- * get_unused_fd_flags(O_CLOEXEC).
- */
-void sync_file_install(struct sync_file *sync_file, int fd);
-
 #ifdef CONFIG_DEBUG_FS
 
 void sync_timeline_debug_add(struct sync_timeline *obj);
diff --git a/drivers/staging/android/sync_debug.c b/drivers/staging/android/sync_debug.c
index 5a7ec58..e4b0e41 100644
--- a/drivers/staging/android/sync_debug.c
+++ b/drivers/staging/android/sync_debug.c
@@ -272,12 +272,12 @@ static long sw_sync_ioctl_create_fence(struct sw_sync_timeline *obj,
 
 	data.fence = fd;
 	if (copy_to_user((void __user *)arg, &data, sizeof(data))) {
-		sync_file_put(sync_file);
+		fput(sync_file->file);
 		err = -EFAULT;
 		goto err;
 	}
 
-	sync_file_install(sync_file, fd);
+	fd_install(fd, sync_file->file);
 
 	return 0;
 
-- 
2.5.5