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: [Linaro-mm-sig] [PATCHv2 4/4] staging: android: ion: Add ioctl to query available heaps

2016-09-02 Thread Laura Abbott 

On 09/02/2016 02:37 PM, Arnd Bergmann wrote:

On Friday, September 2, 2016 2:27:21 PM CEST Laura Abbott wrote:


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?



I don't know a better way that two cast, doing

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

It may help to put this into an inline function though.

Arnd



There already is one it turns out in kernel.h:

#define u64_to_user_ptr(x) (\
{   \
typecheck(u64, x);  \
(void __user *)(uintptr_t)x;\
}   \
)

At least this way I don't have to look at the double casts.

Thanks,
Laura

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

2016-09-02 Thread Laura Abbott 

On 09/02/2016 02:37 PM, Arnd Bergmann wrote:

On Friday, September 2, 2016 2:27:21 PM CEST Laura Abbott wrote:


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?



I don't know a better way that two cast, doing

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

It may help to put this into an inline function though.

Arnd



There already is one it turns out in kernel.h:

#define u64_to_user_ptr(x) (\
{   \
typecheck(u64, x);  \
(void __user *)(uintptr_t)x;\
}   \
)

At least this way I don't have to look at the double casts.

Thanks,
Laura

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

2016-09-02 Thread Arnd Bergmann 
On Friday, September 2, 2016 2:27:21 PM CEST Laura Abbott wrote:
> >
> > 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?
> 

I don't know a better way that two cast, doing

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

It may help to put this into an inline function though.

Arnd

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

2016-09-02 Thread Arnd Bergmann 
On Friday, September 2, 2016 2:27:21 PM CEST Laura Abbott wrote:
> >
> > 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?
> 

I don't know a better way that two cast, doing

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

It may help to put this into an inline function though.

Arnd

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

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

2016-09-01 Thread Laura Abbott 

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;
+   }
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;
+   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;
+   ret = 0;
+   goto out;
+   }
+
+   if (query->cnt <= 0)
+   goto out;
+
+   max_cnt = query->cnt;
+
+   plist_for_each_entry(heap, >heaps, node) {
+   strncpy(hdata.name, heap->name, MAX_HEAP_NAME);
+   hdata.name[sizeof(hdata.name) - 1] = '\0';
+   hdata.type = heap->type;
+   hdata.heap_id = heap->id;
+
+   ret = copy_to_user([cnt],
+  , sizeof(hdata));
+
+   cnt++;
+   if (cnt >= max_cnt)
+   break;
+   }
+
+   query->cnt = cnt;
+out:
+   up_read(>lock);
+   return ret;
+}
+
 static int ion_release(struct inode *inode, struct file *file)
 {
struct ion_client *client = file->private_data;
@@ -1391,6 +1434,7 @@ void ion_device_add_heap(struct ion_device *dev, struct 
ion_heap *heap)
}
}
 
+   dev->heap_cnt++;
up_write(>lock);
 }
 EXPORT_SYMBOL(ion_device_add_heap);
diff --git a/drivers/staging/android/ion/ion_priv.h 
b/drivers/staging/android/ion/ion_priv.h
index 95df6a9..0d0c0aa 100644
--- a/drivers/staging/android/ion/ion_priv.h
+++ b/drivers/staging/android/ion/ion_priv.h
@@ -104,6 +104,7 @@ struct ion_device {
struct dentry *debug_root;
struct dentry *heaps_debug_root;
struct dentry *clients_debug_root;
+   int heap_cnt;
 };
 
 /**
@@ -469,4 +470,6 @@ struct ion_handle *ion_handle_get_by_id(struct ion_client 
*client,
 
 int ion_handle_put(struct ion_handle *handle);
 
+int ion_query_heaps(struct ion_client *client, struct ion_heap_query *query);
+
 #endif /* _ION_PRIV_H */
diff --git a/drivers/staging/android/uapi/ion.h 
b/drivers/staging/android/uapi/ion.h
index 7ca4e8b..112f257 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -137,11 +137,41 @@ struct ion_custom_data {
 
 #define ION_ABI_VERSIONKERNEL_VERSION(0, 1, 0)
 
+#define MAX_HEAP_NAME  32
+
 struct ion_abi_version {
__u32 abi_version;
__u32 reserved;
 };
 
+/**
+ * struct ion_heap_data - data about a heap
+ * @name - first 32 characters of the heap name
+ * @type - heap type
+ * @heap_id - heap id for the heap
+ */

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

2016-09-01 Thread Laura Abbott 

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;
+   }
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;
+   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;
+   ret = 0;
+   goto out;
+   }
+
+   if (query->cnt <= 0)
+   goto out;
+
+   max_cnt = query->cnt;
+
+   plist_for_each_entry(heap, >heaps, node) {
+   strncpy(hdata.name, heap->name, MAX_HEAP_NAME);
+   hdata.name[sizeof(hdata.name) - 1] = '\0';
+   hdata.type = heap->type;
+   hdata.heap_id = heap->id;
+
+   ret = copy_to_user([cnt],
+  , sizeof(hdata));
+
+   cnt++;
+   if (cnt >= max_cnt)
+   break;
+   }
+
+   query->cnt = cnt;
+out:
+   up_read(>lock);
+   return ret;
+}
+
 static int ion_release(struct inode *inode, struct file *file)
 {
struct ion_client *client = file->private_data;
@@ -1391,6 +1434,7 @@ void ion_device_add_heap(struct ion_device *dev, struct 
ion_heap *heap)
}
}
 
+   dev->heap_cnt++;
up_write(>lock);
 }
 EXPORT_SYMBOL(ion_device_add_heap);
diff --git a/drivers/staging/android/ion/ion_priv.h 
b/drivers/staging/android/ion/ion_priv.h
index 95df6a9..0d0c0aa 100644
--- a/drivers/staging/android/ion/ion_priv.h
+++ b/drivers/staging/android/ion/ion_priv.h
@@ -104,6 +104,7 @@ struct ion_device {
struct dentry *debug_root;
struct dentry *heaps_debug_root;
struct dentry *clients_debug_root;
+   int heap_cnt;
 };
 
 /**
@@ -469,4 +470,6 @@ struct ion_handle *ion_handle_get_by_id(struct ion_client 
*client,
 
 int ion_handle_put(struct ion_handle *handle);
 
+int ion_query_heaps(struct ion_client *client, struct ion_heap_query *query);
+
 #endif /* _ION_PRIV_H */
diff --git a/drivers/staging/android/uapi/ion.h 
b/drivers/staging/android/uapi/ion.h
index 7ca4e8b..112f257 100644
--- a/drivers/staging/android/uapi/ion.h
+++ b/drivers/staging/android/uapi/ion.h
@@ -137,11 +137,41 @@ struct ion_custom_data {
 
 #define ION_ABI_VERSIONKERNEL_VERSION(0, 1, 0)
 
+#define MAX_HEAP_NAME  32
+
 struct ion_abi_version {
__u32 abi_version;
__u32 reserved;
 };
 
+/**
+ * struct ion_heap_data - data about a heap
+ * @name - first 32 characters of the heap name
+ * @type - heap type
+ * @heap_id - heap id for the heap
+ */
+struct ion_heap_data {