linux-kernel - Re: [PATCH 1/3] staging: android: remove ION_IOC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7d594698-9b16-e296-1e84-10451e0ced4b@redhat.com>
Date:   Fri, 16 Dec 2016 15:05:54 -0800
From:   Laura Abbott <labbott@...hat.com>
To:     Matthew Smith <matthew.s3h@...il.com>, greg@...ah.com
Cc:     devel@...verdev.osuosl.org, arve@...roid.com,
        riandrews@...roid.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] staging: android: remove ION_IOC_SYNC

On 12/16/2016 02:42 PM, Matthew Smith wrote:
> Remove definition of ION_IOC_CUSTOM from ion.h.
> Remove usage from compat_ion.c and ion-ioctl.c.
> Remove item from TODO file.

The 'remove' from that TODO is more than just removing the code.
There's also an implicit 'replace it with something else'. The
work to do this is still ongoing (see some of my latest patches).
NAK for all 3 of these patches right now.

The patches themselves looked structurally good. For your commit
message next time, explain why the code was being changed, not
just what was changed.

Thanks,
Laura

> 
> Signed-off-by: Matthew Smith <matthew.s3h@...il.com>
> ---
>  drivers/staging/android/TODO             |  3 ---
>  drivers/staging/android/ion/compat_ion.c |  1 -
>  drivers/staging/android/ion/ion-ioctl.c  |  6 ------
>  drivers/staging/android/uapi/ion.h       | 10 ----------
>  4 files changed, 20 deletions(-)
> 
> diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO
> index 8f3ac37..bcf736c 100644
> --- a/drivers/staging/android/TODO
> +++ b/drivers/staging/android/TODO
> @@ -7,9 +7,6 @@ TODO:
>  
>  
>  ion/
> - - Remove ION_IOC_SYNC: Flushing for devices should be purely a kernel internal
> -   interface on top of dma-buf. flush_for_device needs to be added to dma-buf
> -   first.
>   - Remove ION_IOC_CUSTOM: Atm used for cache flushing for cpu access in some
>     vendor trees. Should be replaced with an ioctl on the dma-buf to expose the
>     begin/end_cpu_access hooks to userspace.
> diff --git a/drivers/staging/android/ion/compat_ion.c b/drivers/staging/android/ion/compat_ion.c
> index 9a978d2..b892d3a 100644
> --- a/drivers/staging/android/ion/compat_ion.c
> +++ b/drivers/staging/android/ion/compat_ion.c
> @@ -186,7 +186,6 @@ long compat_ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  	case ION_IOC_SHARE:
>  	case ION_IOC_MAP:
>  	case ION_IOC_IMPORT:
> -	case ION_IOC_SYNC:
>  		return filp->f_op->unlocked_ioctl(filp, cmd,
>  						(unsigned long)compat_ptr(arg));
>  	default:
> diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c
> index 7e7431d..aab086c 100644
> --- a/drivers/staging/android/ion/ion-ioctl.c
> +++ b/drivers/staging/android/ion/ion-ioctl.c
> @@ -51,7 +51,6 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg)
>  static unsigned int ion_ioctl_dir(unsigned int cmd)
>  {
>  	switch (cmd) {
> -	case ION_IOC_SYNC:
>  	case ION_IOC_FREE:
>  	case ION_IOC_CUSTOM:
>  		return _IOC_WRITE;
> @@ -146,11 +145,6 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>  			data.handle.handle = handle->id;
>  		break;
>  	}
> -	case ION_IOC_SYNC:
> -	{
> -		ret = ion_sync_for_device(client, data.fd.fd);
> -		break;
> -	}
>  	case ION_IOC_CUSTOM:
>  	{
>  		if (!dev->custom_ioctl)
> diff --git a/drivers/staging/android/uapi/ion.h b/drivers/staging/android/uapi/ion.h
> index 14cd873..c3a87a5 100644
> --- a/drivers/staging/android/uapi/ion.h
> +++ b/drivers/staging/android/uapi/ion.h
> @@ -207,16 +207,6 @@ struct ion_heap_query {
>  #define ION_IOC_IMPORT		_IOWR(ION_IOC_MAGIC, 5, struct ion_fd_data)
>  
>  /**
> - * DOC: ION_IOC_SYNC - syncs a shared file descriptors to memory
> - *
> - * Deprecated in favor of using the dma_buf api's correctly (syncing
> - * will happen automatically when the buffer is mapped to a device).
> - * If necessary should be used after touching a cached buffer from the cpu,
> - * this will make the buffer in memory coherent.
> - */
> -#define ION_IOC_SYNC		_IOWR(ION_IOC_MAGIC, 7, struct ion_fd_data)
> -
> -/**
>   * DOC: ION_IOC_CUSTOM - call architecture specific ion ioctl
>   *
>   * Takes the argument of the architecture specific ioctl to call and
>