From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754013AbcICMzL (ORCPT ); Sat, 3 Sep 2016 08:55:11 -0400 Received: from mail.linuxfoundation.org ([140.211.169.12]:47513 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751465AbcICMzK (ORCPT ); Sat, 3 Sep 2016 08:55:10 -0400 Date: Sat, 3 Sep 2016 14:55:06 +0200 From: Greg Kroah-Hartman To: Laura Abbott Cc: devel@driverdev.osuosl.org, Jon Medhurst , Android Kernel Team , Jeremy Gebben , Daniel Vetter , Liviu Dudau , Arve =?iso-8859-1?B?SGr4bm5lduVn?= , linux-kernel@vger.kernel.org, linaro-mm-sig@lists.linaro.org, Riley Andrews , John Stultz , Eun Taik Lee , Bryan Huntsman , Sumit Semwal , Chen Feng , Brian Starkey Subject: Re: [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps Message-ID: <20160903125506.GA11457@kroah.com> References: <1472769644-11039-1-git-send-email-labbott@redhat.com> <1472769644-11039-5-git-send-email-labbott@redhat.com> <20160902061410.GC13294@kroah.com> <6e6578da-45c0-00e4-cd99-09b6931abe22@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6e6578da-45c0-00e4-cd99-09b6931abe22@redhat.com> User-Agent: Mutt/1.7.0 (2016-08-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Sep 02, 2016 at 01:41:48PM -0700, Laura Abbott wrote: > On 09/01/2016 11:14 PM, Greg Kroah-Hartman wrote: > > On Thu, Sep 01, 2016 at 03:40:44PM -0700, Laura Abbott wrote: > > > > > > Ion clients currently lack a good method to determine what > > > heaps are available and what ids they map to. This leads > > > to tight coupling between user and kernel space and headaches. > > > Add a query ioctl to let userspace know the availability of > > > heaps. > > > > > > Signed-off-by: Laura Abbott > > > --- > > > drivers/staging/android/ion/ion-ioctl.c | 11 +++++++++ > > > drivers/staging/android/ion/ion.c | 44 +++++++++++++++++++++++++++++++++ > > > drivers/staging/android/ion/ion_priv.h | 3 +++ > > > drivers/staging/android/uapi/ion.h | 39 +++++++++++++++++++++++++++++ > > > 4 files changed, 97 insertions(+) > > > > > > diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c > > > index 53b9520..e76d517 100644 > > > --- a/drivers/staging/android/ion/ion-ioctl.c > > > +++ b/drivers/staging/android/ion/ion-ioctl.c > > > @@ -28,6 +28,7 @@ union ion_ioctl_arg { > > > struct ion_handle_data handle; > > > struct ion_custom_data custom; > > > struct ion_abi_version abi_version; > > > + struct ion_heap_query query; > > > }; > > > > > > static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) > > > @@ -38,6 +39,11 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) > > > case ION_IOC_ABI_VERSION: > > > ret = arg->abi_version.reserved != 0; > > > break; > > > + case ION_IOC_HEAP_QUERY: > > > + ret = arg->query.reserved0 != 0; > > > + ret |= arg->query.reserved1 != 0; > > > + ret |= arg->query.reserved2 != 0; > > > + break; > > > default: > > > break; > > > } > > > @@ -162,6 +168,11 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > > > data.abi_version.abi_version = ION_ABI_VERSION; > > > break; > > > } > > > + case ION_IOC_HEAP_QUERY: > > > + { > > > + ret = ion_query_heaps(client, &data.query); > > > + break; > > > + } > > > > Minor nit, the { } aren't needed here. Yeah, I know the other cases > > have them, but they aren't all needed there either, no need to keep > > copying bad code style :) > > > > Huh, might deserve a checkpatch addition then. Never heard that one before. It's not a checkpatch issue, just a "is this { } even needed" type of an issue :) > > > default: > > > return -ENOTTY; > > > } > > > diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c > > > index 975b48f..91b765c 100644 > > > --- a/drivers/staging/android/ion/ion.c > > > +++ b/drivers/staging/android/ion/ion.c > > > @@ -1174,6 +1174,49 @@ int ion_sync_for_device(struct ion_client *client, int fd) > > > return 0; > > > } > > > > > > +int ion_query_heaps(struct ion_client *client, struct ion_heap_query *query) > > > +{ > > > + struct ion_device *dev = client->dev; > > > + struct ion_heap_data __user *buffer = > > > + (struct ion_heap_data __user *)query->heaps; > > > > Shouldn't query be marked as __user instead of having this cast? > > > > No, the query structure itself is copied into the kernel in ion_ioctl. > The sub field query->heaps is a user pointer which is marked as _u64 > for compatability ala botching-ioctls.txt hence the cast. Ah, ok. Messy :) thanks, greg k-h