* [PATCH v3 0/2] staging: ion: get one device per heap @ 2017-09-20 8:45 Benjamin Gaignard 2017-09-20 8:45 ` [PATCH v3 1/2] staging: ion: simplify ioctl args checking function Benjamin Gaignard 2017-09-20 8:45 ` [PATCH v3 2/2] staging: ion: create one device entry per heap Benjamin Gaignard 0 siblings, 2 replies; 7+ messages in thread From: Benjamin Gaignard @ 2017-09-20 8:45 UTC (permalink / raw) To: labbott, sumit.semwal, gregkh, arve, riandrews, broonie, dan.carpenter Cc: devel, linux-kernel, dri-devel, Benjamin Gaignard version 3: - change ion_device_add_heap prototype to return a possible error version 2: - simplify ioctl check like propose by Dan - make sure that we don't register more than ION_DEV_MAX heaps Until now all ion heaps are addressing using the same device "/dev/ion". This way of working doesn't allow to give access rights (for example with SElinux rules) per heap. This series propose to have one device "/dev/ionX" per heap. Query heaps informations will be possible on each device node but allocation request will only be possible if heap_mask_id match with device minor number. That really change ion ABI because: - device name change - allocation must be done on the correct device/heap. - device major number will not be the same and that could impact init scripts. Even if splitting ion device in multiple node was one of the item of the de-staging TODO list this must be agreed by a larger audience because it is (again) an ion ABI bing bang. Hopefully that could be discussed at next XDC to get a decission on this particular point. Benjamin Gaignard (2): staging: ion: simplify ioctl args checking function staging: ion: create one device entry per heap drivers/staging/android/TODO | 1 - drivers/staging/android/ion/ion-ioctl.c | 20 +++++++++++++------- drivers/staging/android/ion/ion.c | 27 ++++++++++++++++++++------- drivers/staging/android/ion/ion.h | 12 ++++++++---- 4 files changed, 41 insertions(+), 19 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v3 1/2] staging: ion: simplify ioctl args checking function 2017-09-20 8:45 [PATCH v3 0/2] staging: ion: get one device per heap Benjamin Gaignard @ 2017-09-20 8:45 ` Benjamin Gaignard 2017-09-20 8:45 ` [PATCH v3 2/2] staging: ion: create one device entry per heap Benjamin Gaignard 1 sibling, 0 replies; 7+ messages in thread From: Benjamin Gaignard @ 2017-09-20 8:45 UTC (permalink / raw) To: labbott, sumit.semwal, gregkh, arve, riandrews, broonie, dan.carpenter Cc: devel, linux-kernel, dri-devel, Benjamin Gaignard Make arguments checking more easy to read. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org> --- drivers/staging/android/ion/ion-ioctl.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c index d9f8b14..e26b786 100644 --- a/drivers/staging/android/ion/ion-ioctl.c +++ b/drivers/staging/android/ion/ion-ioctl.c @@ -27,19 +27,18 @@ union ion_ioctl_arg { static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) { - int ret = 0; - switch (cmd) { case ION_IOC_HEAP_QUERY: - ret = arg->query.reserved0 != 0; - ret |= arg->query.reserved1 != 0; - ret |= arg->query.reserved2 != 0; + if (arg->query.reserved0 || + arg->query.reserved1 || + arg->query.reserved2 ) + return -EINVAL; break; default: break; } - return ret ? -EINVAL : 0; + return 0; } /* fix up the cases where the ioctl direction bits are incorrect */ -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v3 2/2] staging: ion: create one device entry per heap 2017-09-20 8:45 [PATCH v3 0/2] staging: ion: get one device per heap Benjamin Gaignard 2017-09-20 8:45 ` [PATCH v3 1/2] staging: ion: simplify ioctl args checking function Benjamin Gaignard @ 2017-09-20 8:45 ` Benjamin Gaignard 2017-09-25 18:26 ` Laura Abbott 1 sibling, 1 reply; 7+ messages in thread From: Benjamin Gaignard @ 2017-09-20 8:45 UTC (permalink / raw) To: labbott, sumit.semwal, gregkh, arve, riandrews, broonie, dan.carpenter Cc: devel, linux-kernel, dri-devel, Benjamin Gaignard Instead a getting one common device "/dev/ion" for all the heaps this patch allow to create one device entry ("/dev/ionX") per heap. Getting an entry per heap could allow to set security rules per heap and global ones for all heaps. Allocation requests will be only allowed if the mask_id match with device minor. Query request could be done on any of the devices. Deivce node major will also change and that may impact init scripts. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org> --- version 3: - change ion_device_add_heap prototype to return a possible error version 2: - simplify ioctl check like propose by Dan - make sure that we don't register more than ION_DEV_MAX heaps drivers/staging/android/TODO | 1 - drivers/staging/android/ion/ion-ioctl.c | 11 +++++++++-- drivers/staging/android/ion/ion.c | 27 ++++++++++++++++++++------- drivers/staging/android/ion/ion.h | 12 ++++++++---- 4 files changed, 37 insertions(+), 14 deletions(-) diff --git a/drivers/staging/android/TODO b/drivers/staging/android/TODO index 5f14247..d770ffa 100644 --- a/drivers/staging/android/TODO +++ b/drivers/staging/android/TODO @@ -9,7 +9,6 @@ TODO: ion/ - Add dt-bindings for remaining heaps (chunk and carveout heaps). This would involve putting appropriate bindings in a memory node for Ion to find. - - Split /dev/ion up into multiple nodes (e.g. /dev/ion/heap0) - Better test framework (integration with VGEM was suggested) Please send patches to Greg Kroah-Hartman <greg@kroah.com> and Cc: diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c index e26b786..c8c906c 100644 --- a/drivers/staging/android/ion/ion-ioctl.c +++ b/drivers/staging/android/ion/ion-ioctl.c @@ -25,8 +25,11 @@ union ion_ioctl_arg { struct ion_heap_query query; }; -static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) +static int validate_ioctl_arg(struct file *filp, + unsigned int cmd, union ion_ioctl_arg *arg) { + int mask = 1 << iminor(filp->f_inode); + switch (cmd) { case ION_IOC_HEAP_QUERY: if (arg->query.reserved0 || @@ -34,6 +37,10 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) arg->query.reserved2 ) return -EINVAL; break; + case ION_IOC_ALLOC: + if (!(arg->allocation.heap_id_mask & mask)) + return -EINVAL; + break; default: break; } @@ -69,7 +76,7 @@ long ion_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) if (copy_from_user(&data, (void __user *)arg, _IOC_SIZE(cmd))) return -EFAULT; - ret = validate_ioctl_arg(cmd, &data); + ret = validate_ioctl_arg(filp, cmd, &data); if (WARN_ON_ONCE(ret)) return ret; diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c index 93e2c90..cc2d381 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -40,6 +40,8 @@ #include "ion.h" +#define ION_DEV_MAX 32 + static struct ion_device *internal_dev; static int heap_id; @@ -537,15 +539,28 @@ static int debug_shrink_get(void *data, u64 *val) DEFINE_SIMPLE_ATTRIBUTE(debug_shrink_fops, debug_shrink_get, debug_shrink_set, "%llu\n"); -void ion_device_add_heap(struct ion_heap *heap) +int ion_device_add_heap(struct ion_heap *heap) { struct dentry *debug_file; struct ion_device *dev = internal_dev; + int ret; if (!heap->ops->allocate || !heap->ops->free) pr_err("%s: can not add heap with invalid ops struct.\n", __func__); + if (heap_id >= ION_DEV_MAX) + return -EBUSY; + + heap->ddev.devt = MKDEV(MAJOR(dev->devt), heap_id); + dev_set_name(&heap->ddev, "ion%d", heap_id); + device_initialize(&heap->ddev); + cdev_init(&heap->chrdev, &ion_fops); + heap->chrdev.owner = THIS_MODULE; + ret = cdev_device_add(&heap->chrdev, &heap->ddev); + if (ret < 0) + return ret; + spin_lock_init(&heap->free_lock); heap->free_list_size = 0; @@ -583,6 +598,8 @@ void ion_device_add_heap(struct ion_heap *heap) dev->heap_cnt++; up_write(&dev->lock); + + return 0; } EXPORT_SYMBOL(ion_device_add_heap); @@ -595,13 +612,9 @@ static int ion_device_create(void) if (!idev) return -ENOMEM; - idev->dev.minor = MISC_DYNAMIC_MINOR; - idev->dev.name = "ion"; - idev->dev.fops = &ion_fops; - idev->dev.parent = NULL; - ret = misc_register(&idev->dev); + ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, "ion"); if (ret) { - pr_err("ion: failed to register misc device.\n"); + pr_err("ion: unable to allocate major\n"); kfree(idev); return ret; } diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h index 621e5f7..bb8efc5 100644 --- a/drivers/staging/android/ion/ion.h +++ b/drivers/staging/android/ion/ion.h @@ -17,6 +17,7 @@ #ifndef _ION_H #define _ION_H +#include <linux/cdev.h> #include <linux/device.h> #include <linux/dma-direction.h> #include <linux/kref.h> @@ -26,7 +27,6 @@ #include <linux/sched.h> #include <linux/shrinker.h> #include <linux/types.h> -#include <linux/miscdevice.h> #include "../uapi/ion.h" @@ -90,13 +90,13 @@ void ion_buffer_destroy(struct ion_buffer *buffer); /** * struct ion_device - the metadata of the ion device node - * @dev: the actual misc device + * @dev: the actual device * @buffers: an rb tree of all the existing buffers * @buffer_lock: lock protecting the tree of buffers * @lock: rwsem protecting the tree of heaps and clients */ struct ion_device { - struct miscdevice dev; + dev_t devt; struct rb_root buffers; struct mutex buffer_lock; struct rw_semaphore lock; @@ -152,6 +152,8 @@ struct ion_heap_ops { * struct ion_heap - represents a heap in the system * @node: rb node to put the heap on the device's tree of heaps * @dev: back pointer to the ion_device + * @ddev: device structure + * @chrdev: associated character device * @type: type of heap * @ops: ops struct as above * @flags: flags @@ -176,6 +178,8 @@ struct ion_heap_ops { struct ion_heap { struct plist_node node; struct ion_device *dev; + struct device ddev; + struct cdev chrdev; enum ion_heap_type type; struct ion_heap_ops *ops; unsigned long flags; @@ -212,7 +216,7 @@ bool ion_buffer_fault_user_mappings(struct ion_buffer *buffer); * ion_device_add_heap - adds a heap to the ion device * @heap: the heap to add */ -void ion_device_add_heap(struct ion_heap *heap); +int ion_device_add_heap(struct ion_heap *heap); /** * some helpers for common operations on buffers using the sg_table -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/2] staging: ion: create one device entry per heap 2017-09-20 8:45 ` [PATCH v3 2/2] staging: ion: create one device entry per heap Benjamin Gaignard @ 2017-09-25 18:26 ` Laura Abbott 2017-09-26 5:09 ` Daniel Vetter 0 siblings, 1 reply; 7+ messages in thread From: Laura Abbott @ 2017-09-25 18:26 UTC (permalink / raw) To: Benjamin Gaignard, sumit.semwal, gregkh, arve, riandrews, broonie, dan.carpenter Cc: devel, linux-kernel, dri-devel On 09/20/2017 01:45 AM, Benjamin Gaignard wrote: > Instead a getting one common device "/dev/ion" for > all the heaps this patch allow to create one device > entry ("/dev/ionX") per heap. > Getting an entry per heap could allow to set security rules > per heap and global ones for all heaps. > > Allocation requests will be only allowed if the mask_id > match with device minor. > Query request could be done on any of the devices. > Deivce node major will also change and that may impact init scripts. > We should start Cc linux-api for future changes since we're going to have more than just /dev/ion. Thinking about this some more, I think we need to allow backwards compatibility. It's just not feasible to keep throwing workarounds into userspace if we can avoid it. I'd propose keeping the old /dev/ion misc interface available under a Kconfig and then creating the new split heaps in parallel. On a somewhat related note, there was some interest in possibly having a sysfs interface for Ion long term. I don't think this needs to happen right now but I'd like whatever we do to not make adding that harder. Thanks, Laura ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/2] staging: ion: create one device entry per heap 2017-09-25 18:26 ` Laura Abbott @ 2017-09-26 5:09 ` Daniel Vetter 2017-09-26 6:56 ` Greg KH 0 siblings, 1 reply; 7+ messages in thread From: Daniel Vetter @ 2017-09-26 5:09 UTC (permalink / raw) To: Laura Abbott Cc: Benjamin Gaignard, sumit.semwal, gregkh, arve, riandrews, broonie, dan.carpenter, devel, linux-kernel, dri-devel On Mon, Sep 25, 2017 at 11:26:27AM -0700, Laura Abbott wrote: > On 09/20/2017 01:45 AM, Benjamin Gaignard wrote: > > Instead a getting one common device "/dev/ion" for > > all the heaps this patch allow to create one device > > entry ("/dev/ionX") per heap. > > Getting an entry per heap could allow to set security rules > > per heap and global ones for all heaps. > > > > Allocation requests will be only allowed if the mask_id > > match with device minor. > > Query request could be done on any of the devices. > > Deivce node major will also change and that may impact init scripts. > > > > We should start Cc linux-api for future changes since we're going > to have more than just /dev/ion. > > Thinking about this some more, I think we need to allow backwards > compatibility. It's just not feasible to keep throwing workarounds > into userspace if we can avoid it. I'd propose keeping the old /dev/ion > misc interface available under a Kconfig and then creating the new > split heaps in parallel. > > On a somewhat related note, there was some interest in possibly > having a sysfs interface for Ion long term. I don't think this > needs to happen right now but I'd like whatever we do to not > make adding that harder. Not sure sysfs is a good idea for allocating buffers. The backlight interface is in sysfs, which defacto makes it a root-only interface. Distros really hate that, because it makes unpriviledged X/wayland really hard to pull of. Passing a device file otoh from logind to the compositor is dead simple (and how X et al get at e.g. the drm/input devices already). Adding some links from devices to corresponding ion heaps somewhere in sysfs makes sense, and those could be read by anyone. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/2] staging: ion: create one device entry per heap 2017-09-26 5:09 ` Daniel Vetter @ 2017-09-26 6:56 ` Greg KH 2017-09-26 17:52 ` Laura Abbott 0 siblings, 1 reply; 7+ messages in thread From: Greg KH @ 2017-09-26 6:56 UTC (permalink / raw) To: Laura Abbott, Benjamin Gaignard, sumit.semwal, arve, riandrews, broonie, dan.carpenter, devel, linux-kernel, dri-devel On Tue, Sep 26, 2017 at 07:09:14AM +0200, Daniel Vetter wrote: > On Mon, Sep 25, 2017 at 11:26:27AM -0700, Laura Abbott wrote: > > On 09/20/2017 01:45 AM, Benjamin Gaignard wrote: > > > Instead a getting one common device "/dev/ion" for > > > all the heaps this patch allow to create one device > > > entry ("/dev/ionX") per heap. > > > Getting an entry per heap could allow to set security rules > > > per heap and global ones for all heaps. > > > > > > Allocation requests will be only allowed if the mask_id > > > match with device minor. > > > Query request could be done on any of the devices. > > > Deivce node major will also change and that may impact init scripts. > > > > > > > We should start Cc linux-api for future changes since we're going > > to have more than just /dev/ion. > > > > Thinking about this some more, I think we need to allow backwards > > compatibility. It's just not feasible to keep throwing workarounds > > into userspace if we can avoid it. I'd propose keeping the old /dev/ion > > misc interface available under a Kconfig and then creating the new > > split heaps in parallel. > > > > On a somewhat related note, there was some interest in possibly > > having a sysfs interface for Ion long term. I don't think this > > needs to happen right now but I'd like whatever we do to not > > make adding that harder. > > Not sure sysfs is a good idea for allocating buffers. The backlight > interface is in sysfs, which defacto makes it a root-only interface. > Distros really hate that, because it makes unpriviledged X/wayland really > hard to pull of. Passing a device file otoh from logind to the compositor > is dead simple (and how X et al get at e.g. the drm/input devices > already). sysfs is not a good idea for allocating buffers. We already have some out-of-tree drm drivers doing horrid things in sysfs in ways that totally abuse that api, and vendors have to do crazy things with selinux rules to try to lock it down because of that. A device node is fine, we are used to that for graphics stuff :) thanks, greg k-h ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3 2/2] staging: ion: create one device entry per heap 2017-09-26 6:56 ` Greg KH @ 2017-09-26 17:52 ` Laura Abbott 0 siblings, 0 replies; 7+ messages in thread From: Laura Abbott @ 2017-09-26 17:52 UTC (permalink / raw) To: Greg KH, Benjamin Gaignard, sumit.semwal, arve, riandrews, broonie, dan.carpenter, devel, linux-kernel, dri-devel On 09/25/2017 11:56 PM, Greg KH wrote: > On Tue, Sep 26, 2017 at 07:09:14AM +0200, Daniel Vetter wrote: >> On Mon, Sep 25, 2017 at 11:26:27AM -0700, Laura Abbott wrote: >>> On 09/20/2017 01:45 AM, Benjamin Gaignard wrote: >>>> Instead a getting one common device "/dev/ion" for >>>> all the heaps this patch allow to create one device >>>> entry ("/dev/ionX") per heap. >>>> Getting an entry per heap could allow to set security rules >>>> per heap and global ones for all heaps. >>>> >>>> Allocation requests will be only allowed if the mask_id >>>> match with device minor. >>>> Query request could be done on any of the devices. >>>> Deivce node major will also change and that may impact init scripts. >>>> >>> >>> We should start Cc linux-api for future changes since we're going >>> to have more than just /dev/ion. >>> >>> Thinking about this some more, I think we need to allow backwards >>> compatibility. It's just not feasible to keep throwing workarounds >>> into userspace if we can avoid it. I'd propose keeping the old /dev/ion >>> misc interface available under a Kconfig and then creating the new >>> split heaps in parallel. >>> >>> On a somewhat related note, there was some interest in possibly >>> having a sysfs interface for Ion long term. I don't think this >>> needs to happen right now but I'd like whatever we do to not >>> make adding that harder. >> >> Not sure sysfs is a good idea for allocating buffers. The backlight >> interface is in sysfs, which defacto makes it a root-only interface. >> Distros really hate that, because it makes unpriviledged X/wayland really >> hard to pull of. Passing a device file otoh from logind to the compositor >> is dead simple (and how X et al get at e.g. the drm/input devices >> already). > > sysfs is not a good idea for allocating buffers. We already have some > out-of-tree drm drivers doing horrid things in sysfs in ways that > totally abuse that api, and vendors have to do crazy things with selinux > rules to try to lock it down because of that. A device node is fine, we > are used to that for graphics stuff :) > > thanks, > > greg k-h > I wasn't thinking of sysfs for allocation, this was for exposure of other Ion information that might make more sense than debugfs. Like I said, this was mostly forward thinking to make sure we aren't stuck later. Thanks, Laura ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-09-26 17:52 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-09-20 8:45 [PATCH v3 0/2] staging: ion: get one device per heap Benjamin Gaignard 2017-09-20 8:45 ` [PATCH v3 1/2] staging: ion: simplify ioctl args checking function Benjamin Gaignard 2017-09-20 8:45 ` [PATCH v3 2/2] staging: ion: create one device entry per heap Benjamin Gaignard 2017-09-25 18:26 ` Laura Abbott 2017-09-26 5:09 ` Daniel Vetter 2017-09-26 6:56 ` Greg KH 2017-09-26 17:52 ` Laura Abbott
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox