Re: [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

2016-09-03 Thread Greg Kroah-Hartman 
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, );
> > > + 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

Re: [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

2016-09-03 Thread Greg Kroah-Hartman 
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, );
> > > + 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

Re: [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

2016-09-02 Thread Laura Abbott 

On 09/01/2016 04:44 PM, kbuild test robot wrote:

Hi Laura,

[auto build test WARNING on next-20160825]
[cannot apply to staging/staging-testing v4.8-rc4 v4.8-rc3 v4.8-rc2 v4.8-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]
[Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto for 
convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:
https://github.com/0day-ci/linux/commits/Laura-Abbott/staging-android-ion-Drop-heap-type-masks/20160902-071022
config: mips-allyesconfig (attached as .config)
compiler: mips-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=mips

All warnings (new ones prefixed by >>):

   drivers/staging/android/ion/ion.c: In function 'ion_query_heaps':

drivers/staging/android/ion/ion.c:1181:3: warning: cast to pointer from integer 
of different size [-Wint-to-pointer-cast]

  (struct ion_heap_data __user *)query->heaps;
  ^



botching-up-ioctls.txt says:

 * Pointers are __u64, cast from/to a uintprt_t on the userspace side and
   from/to a void __user * in the kernel. Try really hard not to delay this
   conversion or worse, fiddle the raw __u64 through your code since that
   diminishes the checking tools like sparse can provide.

This doesn't seem like it will work on 32-bit systems since you'll end up
with the warning above. Is there a better option or did I misunderstand
how that is supposed to work?

Thanks,
Laura

Re: [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

2016-09-02 Thread Laura Abbott 

On 09/01/2016 04:44 PM, kbuild test robot wrote:

Hi Laura,

[auto build test WARNING on next-20160825]
[cannot apply to staging/staging-testing v4.8-rc4 v4.8-rc3 v4.8-rc2 v4.8-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]
[Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto for 
convenience) to record what (public, well-known) commit your patch series was built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:
https://github.com/0day-ci/linux/commits/Laura-Abbott/staging-android-ion-Drop-heap-type-masks/20160902-071022
config: mips-allyesconfig (attached as .config)
compiler: mips-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=mips

All warnings (new ones prefixed by >>):

   drivers/staging/android/ion/ion.c: In function 'ion_query_heaps':

drivers/staging/android/ion/ion.c:1181:3: warning: cast to pointer from integer 
of different size [-Wint-to-pointer-cast]

  (struct ion_heap_data __user *)query->heaps;
  ^



botching-up-ioctls.txt says:

 * Pointers are __u64, cast from/to a uintprt_t on the userspace side and
   from/to a void __user * in the kernel. Try really hard not to delay this
   conversion or worse, fiddle the raw __u64 through your code since that
   diminishes the checking tools like sparse can provide.

This doesn't seem like it will work on 32-bit systems since you'll end up
with the warning above. Is there a better option or did I misunderstand
how that is supposed to work?

Thanks,
Laura

Re: [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

2016-09-02 Thread Laura Abbott 

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, );
+   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.





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.


+   int ret = -EINVAL, cnt = 0, max_cnt;
+   struct ion_heap *heap;
+   struct ion_heap_data hdata;
+
+   memset(, 0, sizeof(hdata));
+
+   down_read(>lock);
+   if (!buffer) {
+   query->cnt = dev->heap_cnt;


Wait, query is __user?

I think the mixing of user/kernel pointers here isn't quite right, or I
just really can't figure it out...

And kbuild didn't seem to like this patch either :(

But your first 2 patches are great, I'll queue them up later today.

thanks,

greg k-h

Re: [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

2016-09-02 Thread Laura Abbott 

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, );
+   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.





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.


+   int ret = -EINVAL, cnt = 0, max_cnt;
+   struct ion_heap *heap;
+   struct ion_heap_data hdata;
+
+   memset(, 0, sizeof(hdata));
+
+   down_read(>lock);
+   if (!buffer) {
+   query->cnt = dev->heap_cnt;


Wait, query is __user?

I think the mixing of user/kernel pointers here isn't quite right, or I
just really can't figure it out...

And kbuild didn't seem to like this patch either :(

But your first 2 patches are great, I'll queue them up later today.

thanks,

greg k-h

Re: [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

2016-09-02 Thread Greg Kroah-Hartman 
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, );
> + 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 :)



>   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?

> + int ret = -EINVAL, cnt = 0, max_cnt;
> + struct ion_heap *heap;
> + struct ion_heap_data hdata;
> +
> + memset(, 0, sizeof(hdata));
> +
> + down_read(>lock);
> + if (!buffer) {
> + query->cnt = dev->heap_cnt;

Wait, query is __user?

I think the mixing of user/kernel pointers here isn't quite right, or I
just really can't figure it out...

And kbuild didn't seem to like this patch either :(

But your first 2 patches are great, I'll queue them up later today.

thanks,

greg k-h

Re: [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

2016-09-02 Thread Greg Kroah-Hartman 
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, );
> + 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 :)



>   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?

> + int ret = -EINVAL, cnt = 0, max_cnt;
> + struct ion_heap *heap;
> + struct ion_heap_data hdata;
> +
> + memset(, 0, sizeof(hdata));
> +
> + down_read(>lock);
> + if (!buffer) {
> + query->cnt = dev->heap_cnt;

Wait, query is __user?

I think the mixing of user/kernel pointers here isn't quite right, or I
just really can't figure it out...

And kbuild didn't seem to like this patch either :(

But your first 2 patches are great, I'll queue them up later today.

thanks,

greg k-h

Re: [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

2016-09-01 Thread kbuild test robot 
Hi Laura,

[auto build test WARNING on next-20160825]
[cannot apply to staging/staging-testing v4.8-rc4 v4.8-rc3 v4.8-rc2 v4.8-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]
[Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto for 
convenience) to record what (public, well-known) commit your patch series was 
built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:
https://github.com/0day-ci/linux/commits/Laura-Abbott/staging-android-ion-Drop-heap-type-masks/20160902-071022
config: mips-allyesconfig (attached as .config)
compiler: mips-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=mips 

All warnings (new ones prefixed by >>):

   drivers/staging/android/ion/ion.c: In function 'ion_query_heaps':
>> drivers/staging/android/ion/ion.c:1181:3: warning: cast to pointer from 
>> integer of different size [-Wint-to-pointer-cast]
  (struct ion_heap_data __user *)query->heaps;
  ^

vim +1181 drivers/staging/android/ion/ion.c

  1165 __func__);
  1166  dma_buf_put(dmabuf);
  1167  return -EINVAL;
  1168  }
  1169  buffer = dmabuf->priv;
  1170  
  1171  dma_sync_sg_for_device(NULL, buffer->sg_table->sgl,
  1172 buffer->sg_table->nents, 
DMA_BIDIRECTIONAL);
  1173  dma_buf_put(dmabuf);
  1174  return 0;
  1175  }
  1176  
  1177  int ion_query_heaps(struct ion_client *client, struct ion_heap_query 
*query)
  1178  {
  1179  struct ion_device *dev = client->dev;
  1180  struct ion_heap_data __user *buffer =
> 1181  (struct ion_heap_data __user *)query->heaps;
  1182  int ret = -EINVAL, cnt = 0, max_cnt;
  1183  struct ion_heap *heap;
  1184  struct ion_heap_data hdata;
  1185  
  1186  memset(, 0, sizeof(hdata));
  1187  
  1188  down_read(>lock);
  1189  if (!buffer) {

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data

Re: [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

2016-09-01 Thread kbuild test robot 
Hi Laura,

[auto build test WARNING on next-20160825]
[cannot apply to staging/staging-testing v4.8-rc4 v4.8-rc3 v4.8-rc2 v4.8-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]
[Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto for 
convenience) to record what (public, well-known) commit your patch series was 
built on]
[Check https://git-scm.com/docs/git-format-patch for more information]

url:
https://github.com/0day-ci/linux/commits/Laura-Abbott/staging-android-ion-Drop-heap-type-masks/20160902-071022
config: mips-allyesconfig (attached as .config)
compiler: mips-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=mips 

All warnings (new ones prefixed by >>):

   drivers/staging/android/ion/ion.c: In function 'ion_query_heaps':
>> drivers/staging/android/ion/ion.c:1181:3: warning: cast to pointer from 
>> integer of different size [-Wint-to-pointer-cast]
  (struct ion_heap_data __user *)query->heaps;
  ^

vim +1181 drivers/staging/android/ion/ion.c

  1165 __func__);
  1166  dma_buf_put(dmabuf);
  1167  return -EINVAL;
  1168  }
  1169  buffer = dmabuf->priv;
  1170  
  1171  dma_sync_sg_for_device(NULL, buffer->sg_table->sgl,
  1172 buffer->sg_table->nents, 
DMA_BIDIRECTIONAL);
  1173  dma_buf_put(dmabuf);
  1174  return 0;
  1175  }
  1176  
  1177  int ion_query_heaps(struct ion_client *client, struct ion_heap_query 
*query)
  1178  {
  1179  struct ion_device *dev = client->dev;
  1180  struct ion_heap_data __user *buffer =
> 1181  (struct ion_heap_data __user *)query->heaps;
  1182  int ret = -EINVAL, cnt = 0, max_cnt;
  1183  struct ion_heap *heap;
  1184  struct ion_heap_data hdata;
  1185  
  1186  memset(, 0, sizeof(hdata));
  1187  
  1188  down_read(>lock);
  1189  if (!buffer) {

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: Binary data