On 03/06/2017 09:00 AM, Emil Velikov wrote:

On 6 March 2017 at 10:29, Daniel Vetter daniel@ffwll.ch wrote:

...

On Fri, Mar 03, 2017 at 10:46:03AM -0800, Laura Abbott wrote:

...

On 03/03/2017 08:39 AM, Laurent Pinchart wrote:

...

Hi Daniel,

On Friday 03 Mar 2017 10:56:54 Daniel Vetter wrote:

...

On Thu, Mar 02, 2017 at 01:44:38PM -0800, Laura Abbott wrote:

...

Now that we call dma_map in the dma_buf API callbacks there is no need to use the existing cache APIs. Remove the sync ioctl and the existing bad dma_sync calls. Explicit caching can be handled with the dma_buf sync API.

Signed-off-by: Laura Abbott labbott@redhat.com

drivers/staging/android/ion/ion-ioctl.c | 5 ---- drivers/staging/android/ion/ion.c | 40 -------------------- drivers/staging/android/ion/ion_carveout_heap.c | 6 ---- drivers/staging/android/ion/ion_chunk_heap.c | 6 ---- drivers/staging/android/ion/ion_page_pool.c | 3 -- drivers/staging/android/ion/ion_system_heap.c | 5 ---- 6 files changed, 65 deletions(-)

diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c index 5b2e93f..f820d77 100644 --- a/drivers/staging/android/ion/ion-ioctl.c +++ b/drivers/staging/android/ion/ion-ioctl.c @@ -146,11 +146,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;
  

• }

You missed the case ION_IOC_SYNC: in compat_ion.c.

While at it: Should we also remove the entire custom_ioctl infrastructure? It's entirely unused afaict, and for a pure buffer allocator I don't see any need to have custom ioctl.

I second that, if you want to make ion a standard API, then we certainly don't want any custom ioctl.

...

More code to remove potentially:

• The entire compat ioctl stuff - would be an abi break, but I guess if we pick the 32bit abi and clean up the uapi headers we'll be mostly fine. would allow us to remove compat_ion.c entirely.

• ION_IOC_IMPORT: With this ion is purely an allocator, so not sure we still need to be able to import anything. All the cache flushing/mapping is done through dma-buf ops/ioctls.

Good point to all of the above. I was considering keeping the import around for backwards compatibility reasons but given how much other stuff is being potentially broken, everything should just get ripped out.

If you're ok with breaking the world, then I strongly suggest we go through the uapi header and replace all types with the standard fixed-width ones (__s32, __s64 and __u32, __u64). Allows us to remove all the compat ioctl code :-)

I think the other comments from your "botching-up ioctls" [1] also apply ;-) Namely - align structs to multiple of 64bit, add "flags" and properly verity user input returning -EINVAL.

-Emil

[1] https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documen...

I'm more torn on this. There's a difference between dropping an old ioctl/implicit caching vs. changing an actual ioctl ABI. Maybe having obvious breakage is better than subtle though, plus nobody has come begging me not to break the ABI yet. I might leave this for right before we do the actual move out of staging.

Thanks, Laura