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