* [PATCH v6 0/2] staging: ion: get one device per heap @ 2017-10-23 15:55 Benjamin Gaignard 2017-10-23 15:55 ` [PATCH v6 1/2] staging: ion: simplify ioctl args checking function Benjamin Gaignard 2017-10-23 15:55 ` [PATCH v6 2/2] staging: ion: create one device entry per heap Benjamin Gaignard 0 siblings, 2 replies; 12+ messages in thread From: Benjamin Gaignard @ 2017-10-23 15:55 UTC (permalink / raw) To: labbott, sumit.semwal, gregkh, arve, riandrews, broonie, dan.carpenter Cc: devel, linux-kernel, dri-devel, linux-api, Benjamin Gaignard version 6: - add platform_bus and platform_bus_type when register Ion devices to make them appear in /sys/devices/platform. - re-order include files in aphabetic order. version 5: - create a configuration flag to keep legacy Ion misc device version 4: - add a configuration flag to switch between legacy Ion misc device and one device per heap version. This change has been suggested after Laura talks at XDC 2017. 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. Using legacy Ion misc device is still by setting ION_LEGACY_DEVICE_API configuration flag. 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/Kconfig | 7 +++++ drivers/staging/android/ion/ion-ioctl.c | 29 +++++++++++++----- drivers/staging/android/ion/ion.c | 53 ++++++++++++++++++++++++++------- drivers/staging/android/ion/ion.h | 15 ++++++++-- 5 files changed, 84 insertions(+), 21 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v6 1/2] staging: ion: simplify ioctl args checking function 2017-10-23 15:55 [PATCH v6 0/2] staging: ion: get one device per heap Benjamin Gaignard @ 2017-10-23 15:55 ` Benjamin Gaignard 2017-10-23 15:55 ` [PATCH v6 2/2] staging: ion: create one device entry per heap Benjamin Gaignard 1 sibling, 0 replies; 12+ messages in thread From: Benjamin Gaignard @ 2017-10-23 15:55 UTC (permalink / raw) To: labbott, sumit.semwal, gregkh, arve, riandrews, broonie, dan.carpenter Cc: devel, linux-kernel, dri-devel, linux-api, Benjamin Gaignard Make arguments checking more easy to read. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org> Acked-by: Laura Abbott <labbott@redhat.com> --- 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] 12+ messages in thread
* [PATCH v6 2/2] staging: ion: create one device entry per heap 2017-10-23 15:55 [PATCH v6 0/2] staging: ion: get one device per heap Benjamin Gaignard 2017-10-23 15:55 ` [PATCH v6 1/2] staging: ion: simplify ioctl args checking function Benjamin Gaignard @ 2017-10-23 15:55 ` Benjamin Gaignard 2017-10-24 16:14 ` Jordan Crouse 2017-10-31 19:03 ` Laura Abbott 1 sibling, 2 replies; 12+ messages in thread From: Benjamin Gaignard @ 2017-10-23 15:55 UTC (permalink / raw) To: labbott, sumit.semwal, gregkh, arve, riandrews, broonie, dan.carpenter Cc: devel, linux-kernel, dri-devel, linux-api, Benjamin Gaignard Instead a getting only 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. Ion devices are parentless so it is need to add platform_bus as parent and platform_bus_type as bus to be put in /sys/device/paltform. Those two parameters need platform_device.h to be included but include files weren't in alphabetic order so I reorder them correctly. Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org> --- drivers/staging/android/TODO | 1 - drivers/staging/android/ion/Kconfig | 7 +++++ drivers/staging/android/ion/ion-ioctl.c | 18 +++++++++-- drivers/staging/android/ion/ion.c | 53 ++++++++++++++++++++++++++------- drivers/staging/android/ion/ion.h | 15 ++++++++-- 5 files changed, 79 insertions(+), 15 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/Kconfig b/drivers/staging/android/ion/Kconfig index a517b2d..cb4666e 100644 --- a/drivers/staging/android/ion/Kconfig +++ b/drivers/staging/android/ion/Kconfig @@ -10,6 +10,13 @@ menuconfig ION If you're not using Android its probably safe to say N here. +config ION_LEGACY_DEVICE_API + bool "Keep using Ion legacy misc device API" + depends on ION + help + Choose this option to keep using Ion legacy misc device API + i.e. /dev/ion + config ION_SYSTEM_HEAP bool "Ion system heap" depends on ION diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c index e26b786..bb5c77b 100644 --- a/drivers/staging/android/ion/ion-ioctl.c +++ b/drivers/staging/android/ion/ion-ioctl.c @@ -25,7 +25,8 @@ 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) { switch (cmd) { case ION_IOC_HEAP_QUERY: @@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) arg->query.reserved2 ) return -EINVAL; break; + + case ION_IOC_ALLOC: + { + int mask = 1 << iminor(filp->f_inode); + +#ifdef CONFIG_ION_LEGACY_DEVICE_API + if (imajor(filp->f_inode) == MISC_MAJOR) + return 0; +#endif + if (!(arg->allocation.heap_id_mask & mask)) + return -EINVAL; + break; + } default: break; } @@ -69,7 +83,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..dd66f55 100644 --- a/drivers/staging/android/ion/ion.c +++ b/drivers/staging/android/ion/ion.c @@ -15,31 +15,35 @@ * */ +#include <linux/anon_inodes.h> +#include <linux/debugfs.h> #include <linux/device.h> +#include <linux/dma-buf.h> #include <linux/err.h> +#include <linux/export.h> #include <linux/file.h> #include <linux/freezer.h> #include <linux/fs.h> -#include <linux/anon_inodes.h> +#include <linux/idr.h> #include <linux/kthread.h> #include <linux/list.h> #include <linux/memblock.h> #include <linux/miscdevice.h> -#include <linux/export.h> #include <linux/mm.h> #include <linux/mm_types.h> +#include <linux/platform_device.h> #include <linux/rbtree.h> -#include <linux/slab.h> +#include <linux/sched/task.h> #include <linux/seq_file.h> +#include <linux/slab.h> #include <linux/uaccess.h> #include <linux/vmalloc.h> -#include <linux/debugfs.h> -#include <linux/dma-buf.h> -#include <linux/idr.h> -#include <linux/sched/task.h> #include "ion.h" +#define ION_DEV_MAX 32 +#define ION_NAME "ion" + static struct ion_device *internal_dev; static int heap_id; @@ -537,15 +541,30 @@ 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 = 0; 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.parent = &platform_bus; + heap->ddev.bus = &platform_bus_type; + heap->ddev.devt = MKDEV(MAJOR(internal_dev->devt), heap_id); + dev_set_name(&heap->ddev, ION_NAME"%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 +602,8 @@ void ion_device_add_heap(struct ion_heap *heap) dev->heap_cnt++; up_write(&dev->lock); + + return ret; } EXPORT_SYMBOL(ion_device_add_heap); @@ -595,8 +616,9 @@ static int ion_device_create(void) if (!idev) return -ENOMEM; +#ifdef CONFIG_ION_LEGACY_DEVICE_API idev->dev.minor = MISC_DYNAMIC_MINOR; - idev->dev.name = "ion"; + idev->dev.name = ION_NAME; idev->dev.fops = &ion_fops; idev->dev.parent = NULL; ret = misc_register(&idev->dev); @@ -605,8 +627,19 @@ static int ion_device_create(void) kfree(idev); return ret; } +#endif + + ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, ION_NAME); + if (ret) { + pr_err("ion: unable to allocate device\n"); +#ifdef CONFIG_ION_LEGACY_DEVICE_API + misc_deregister(&idev->dev); +#endif + kfree(idev); + return ret; + } - idev->debug_root = debugfs_create_dir("ion", NULL); + idev->debug_root = debugfs_create_dir(ION_NAME, NULL); if (!idev->debug_root) { pr_err("ion: failed to create debugfs root directory.\n"); goto debugfs_done; diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h index 621e5f7..2b00ccb 100644 --- a/drivers/staging/android/ion/ion.h +++ b/drivers/staging/android/ion/ion.h @@ -17,16 +17,19 @@ #ifndef _ION_H #define _ION_H +#include <linux/cdev.h> #include <linux/device.h> #include <linux/dma-direction.h> #include <linux/kref.h> +#ifdef CONFIG_ION_LEGACY_DEVICE_API +#include <linux/miscdevice.h> +#endif #include <linux/mm_types.h> #include <linux/mutex.h> #include <linux/rbtree.h> #include <linux/sched.h> #include <linux/shrinker.h> #include <linux/types.h> -#include <linux/miscdevice.h> #include "../uapi/ion.h" @@ -91,12 +94,16 @@ void ion_buffer_destroy(struct ion_buffer *buffer); /** * struct ion_device - the metadata of the ion device node * @dev: the actual misc device + * @devt: Ion 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 { +#ifdef CONFIG_ION_LEGACY_DEVICE_API struct miscdevice dev; +#endif + dev_t devt; struct rb_root buffers; struct mutex buffer_lock; struct rw_semaphore lock; @@ -152,6 +159,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 +185,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 +223,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] 12+ messages in thread
* Re: [PATCH v6 2/2] staging: ion: create one device entry per heap 2017-10-23 15:55 ` [PATCH v6 2/2] staging: ion: create one device entry per heap Benjamin Gaignard @ 2017-10-24 16:14 ` Jordan Crouse 2017-10-31 19:03 ` Laura Abbott 1 sibling, 0 replies; 12+ messages in thread From: Jordan Crouse @ 2017-10-24 16:14 UTC (permalink / raw) To: Benjamin Gaignard Cc: labbott, sumit.semwal, gregkh, arve, riandrews, broonie, dan.carpenter, devel, linux-api, linux-kernel, dri-devel On Mon, Oct 23, 2017 at 05:55:37PM +0200, Benjamin Gaignard wrote: > Instead a getting only 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. > > Ion devices are parentless so it is need to add platform_bus as > parent and platform_bus_type as bus to be put in /sys/device/paltform. nit: paltform -> platform. Jordan -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6 2/2] staging: ion: create one device entry per heap 2017-10-23 15:55 ` [PATCH v6 2/2] staging: ion: create one device entry per heap Benjamin Gaignard 2017-10-24 16:14 ` Jordan Crouse @ 2017-10-31 19:03 ` Laura Abbott 2017-10-31 19:11 ` Mark Brown 1 sibling, 1 reply; 12+ messages in thread From: Laura Abbott @ 2017-10-31 19:03 UTC (permalink / raw) To: Benjamin Gaignard, sumit.semwal, gregkh, arve, riandrews, broonie, dan.carpenter Cc: devel, linux-kernel, dri-devel, linux-api On 10/23/2017 08:55 AM, Benjamin Gaignard wrote: > Instead a getting only 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. > I'm wondering if we should always keep /dev/ion for the query ioctl and just disallow allocation from /dev/ion. I guess running the query ioctl on /dev/ion0 always wouldn't be too bad? Anyone else have strong opinions? > Ion devices are parentless so it is need to add platform_bus as > parent and platform_bus_type as bus to be put in /sys/device/paltform. > Those two parameters need platform_device.h to be included but > include files weren't in alphabetic order so I reorder them correctly. > I'm not a fan of the platform bus but I have mixed feelings about creating a dedicated bus type. I guess if we really need a bus type we can do it later? Thanks, Laura > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@linaro.org> > --- > drivers/staging/android/TODO | 1 - > drivers/staging/android/ion/Kconfig | 7 +++++ > drivers/staging/android/ion/ion-ioctl.c | 18 +++++++++-- > drivers/staging/android/ion/ion.c | 53 ++++++++++++++++++++++++++------- > drivers/staging/android/ion/ion.h | 15 ++++++++-- > 5 files changed, 79 insertions(+), 15 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/Kconfig b/drivers/staging/android/ion/Kconfig > index a517b2d..cb4666e 100644 > --- a/drivers/staging/android/ion/Kconfig > +++ b/drivers/staging/android/ion/Kconfig > @@ -10,6 +10,13 @@ menuconfig ION > If you're not using Android its probably safe to > say N here. > > +config ION_LEGACY_DEVICE_API > + bool "Keep using Ion legacy misc device API" > + depends on ION > + help > + Choose this option to keep using Ion legacy misc device API > + i.e. /dev/ion > + > config ION_SYSTEM_HEAP > bool "Ion system heap" > depends on ION > diff --git a/drivers/staging/android/ion/ion-ioctl.c b/drivers/staging/android/ion/ion-ioctl.c > index e26b786..bb5c77b 100644 > --- a/drivers/staging/android/ion/ion-ioctl.c > +++ b/drivers/staging/android/ion/ion-ioctl.c > @@ -25,7 +25,8 @@ 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) > { > switch (cmd) { > case ION_IOC_HEAP_QUERY: > @@ -34,6 +35,19 @@ static int validate_ioctl_arg(unsigned int cmd, union ion_ioctl_arg *arg) > arg->query.reserved2 ) > return -EINVAL; > break; > + > + case ION_IOC_ALLOC: > + { > + int mask = 1 << iminor(filp->f_inode); > + > +#ifdef CONFIG_ION_LEGACY_DEVICE_API > + if (imajor(filp->f_inode) == MISC_MAJOR) > + return 0; > +#endif > + if (!(arg->allocation.heap_id_mask & mask)) > + return -EINVAL; > + break; > + } > default: > break; > } > @@ -69,7 +83,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..dd66f55 100644 > --- a/drivers/staging/android/ion/ion.c > +++ b/drivers/staging/android/ion/ion.c > @@ -15,31 +15,35 @@ > * > */ > > +#include <linux/anon_inodes.h> > +#include <linux/debugfs.h> > #include <linux/device.h> > +#include <linux/dma-buf.h> > #include <linux/err.h> > +#include <linux/export.h> > #include <linux/file.h> > #include <linux/freezer.h> > #include <linux/fs.h> > -#include <linux/anon_inodes.h> > +#include <linux/idr.h> > #include <linux/kthread.h> > #include <linux/list.h> > #include <linux/memblock.h> > #include <linux/miscdevice.h> > -#include <linux/export.h> > #include <linux/mm.h> > #include <linux/mm_types.h> > +#include <linux/platform_device.h> > #include <linux/rbtree.h> > -#include <linux/slab.h> > +#include <linux/sched/task.h> > #include <linux/seq_file.h> > +#include <linux/slab.h> > #include <linux/uaccess.h> > #include <linux/vmalloc.h> > -#include <linux/debugfs.h> > -#include <linux/dma-buf.h> > -#include <linux/idr.h> > -#include <linux/sched/task.h> > > #include "ion.h" > > +#define ION_DEV_MAX 32 > +#define ION_NAME "ion" > + > static struct ion_device *internal_dev; > static int heap_id; > > @@ -537,15 +541,30 @@ 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 = 0; > > 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.parent = &platform_bus; > + heap->ddev.bus = &platform_bus_type; > + heap->ddev.devt = MKDEV(MAJOR(internal_dev->devt), heap_id); > + dev_set_name(&heap->ddev, ION_NAME"%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 +602,8 @@ void ion_device_add_heap(struct ion_heap *heap) > > dev->heap_cnt++; > up_write(&dev->lock); > + > + return ret; > } > EXPORT_SYMBOL(ion_device_add_heap); > > @@ -595,8 +616,9 @@ static int ion_device_create(void) > if (!idev) > return -ENOMEM; > > +#ifdef CONFIG_ION_LEGACY_DEVICE_API > idev->dev.minor = MISC_DYNAMIC_MINOR; > - idev->dev.name = "ion"; > + idev->dev.name = ION_NAME; > idev->dev.fops = &ion_fops; > idev->dev.parent = NULL; > ret = misc_register(&idev->dev); > @@ -605,8 +627,19 @@ static int ion_device_create(void) > kfree(idev); > return ret; > } > +#endif > + > + ret = alloc_chrdev_region(&idev->devt, 0, ION_DEV_MAX, ION_NAME); > + if (ret) { > + pr_err("ion: unable to allocate device\n"); > +#ifdef CONFIG_ION_LEGACY_DEVICE_API > + misc_deregister(&idev->dev); > +#endif > + kfree(idev); > + return ret; > + } > > - idev->debug_root = debugfs_create_dir("ion", NULL); > + idev->debug_root = debugfs_create_dir(ION_NAME, NULL); > if (!idev->debug_root) { > pr_err("ion: failed to create debugfs root directory.\n"); > goto debugfs_done; > diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h > index 621e5f7..2b00ccb 100644 > --- a/drivers/staging/android/ion/ion.h > +++ b/drivers/staging/android/ion/ion.h > @@ -17,16 +17,19 @@ > #ifndef _ION_H > #define _ION_H > > +#include <linux/cdev.h> > #include <linux/device.h> > #include <linux/dma-direction.h> > #include <linux/kref.h> > +#ifdef CONFIG_ION_LEGACY_DEVICE_API > +#include <linux/miscdevice.h> > +#endif > #include <linux/mm_types.h> > #include <linux/mutex.h> > #include <linux/rbtree.h> > #include <linux/sched.h> > #include <linux/shrinker.h> > #include <linux/types.h> > -#include <linux/miscdevice.h> > > #include "../uapi/ion.h" > > @@ -91,12 +94,16 @@ void ion_buffer_destroy(struct ion_buffer *buffer); > /** > * struct ion_device - the metadata of the ion device node > * @dev: the actual misc device > + * @devt: Ion 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 { > +#ifdef CONFIG_ION_LEGACY_DEVICE_API > struct miscdevice dev; > +#endif > + dev_t devt; > struct rb_root buffers; > struct mutex buffer_lock; > struct rw_semaphore lock; > @@ -152,6 +159,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 +185,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 +223,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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6 2/2] staging: ion: create one device entry per heap 2017-10-31 19:03 ` Laura Abbott @ 2017-10-31 19:11 ` Mark Brown 2017-10-31 19:45 ` Laura Abbott 2017-11-02 10:44 ` Greg KH 0 siblings, 2 replies; 12+ messages in thread From: Mark Brown @ 2017-10-31 19:11 UTC (permalink / raw) To: Laura Abbott Cc: Benjamin Gaignard, sumit.semwal, gregkh, arve, riandrews, dan.carpenter, devel, linux-kernel, dri-devel, linux-api [-- Attachment #1: Type: text/plain, Size: 648 bytes --] On Tue, Oct 31, 2017 at 12:03:35PM -0700, Laura Abbott wrote: > I'm not a fan of the platform bus but I have mixed feelings about > creating a dedicated bus type. I guess if we really need a bus > type we can do it later? There was a discussion a while ago in the context of I2C/SPI MFDs which concluded that if you need a bus and it's going to be effectively noop then you should just use the platform bus as anything else will consist almost entirely of cut'n'paste from the platform bus with some light sed usage and code duplication is bad. It's not super lovely as it's not actually a memory mapped device but it's the best idea we've got. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6 2/2] staging: ion: create one device entry per heap 2017-10-31 19:11 ` Mark Brown @ 2017-10-31 19:45 ` Laura Abbott 2017-11-02 10:44 ` Greg KH 1 sibling, 0 replies; 12+ messages in thread From: Laura Abbott @ 2017-10-31 19:45 UTC (permalink / raw) To: Mark Brown Cc: Benjamin Gaignard, sumit.semwal, gregkh, arve, riandrews, dan.carpenter, devel, linux-kernel, dri-devel, linux-api On 10/31/2017 12:11 PM, Mark Brown wrote: > On Tue, Oct 31, 2017 at 12:03:35PM -0700, Laura Abbott wrote: > >> I'm not a fan of the platform bus but I have mixed feelings about >> creating a dedicated bus type. I guess if we really need a bus >> type we can do it later? > > There was a discussion a while ago in the context of I2C/SPI MFDs > which concluded that if you need a bus and it's going to be effectively > noop then you should just use the platform bus as anything else will > consist almost entirely of cut'n'paste from the platform bus with some > light sed usage and code duplication is bad. It's not super lovely as > it's not actually a memory mapped device but it's the best idea we've > got. > Thanks for the pointer. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6 2/2] staging: ion: create one device entry per heap 2017-10-31 19:11 ` Mark Brown 2017-10-31 19:45 ` Laura Abbott @ 2017-11-02 10:44 ` Greg KH 2017-11-02 11:10 ` Mark Brown 1 sibling, 1 reply; 12+ messages in thread From: Greg KH @ 2017-11-02 10:44 UTC (permalink / raw) To: Mark Brown Cc: Laura Abbott, Benjamin Gaignard, sumit.semwal, arve, riandrews, dan.carpenter, devel, linux-kernel, dri-devel, linux-api On Tue, Oct 31, 2017 at 07:11:53PM +0000, Mark Brown wrote: > On Tue, Oct 31, 2017 at 12:03:35PM -0700, Laura Abbott wrote: > > > I'm not a fan of the platform bus but I have mixed feelings about > > creating a dedicated bus type. I guess if we really need a bus > > type we can do it later? > > There was a discussion a while ago in the context of I2C/SPI MFDs > which concluded that if you need a bus and it's going to be effectively > noop then you should just use the platform bus as anything else will > consist almost entirely of cut'n'paste from the platform bus with some > light sed usage and code duplication is bad. It's not super lovely as > it's not actually a memory mapped device but it's the best idea we've > got. Ugh, I hate that. What's wrong with using a "virtual" device instead? I can create a "virtual" bus for things like this if they really want a "simple" bus, abusing platform for this is the major reason I hate the platform bus code... thanks, greg k-h ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6 2/2] staging: ion: create one device entry per heap 2017-11-02 10:44 ` Greg KH @ 2017-11-02 11:10 ` Mark Brown 2017-11-06 14:42 ` Benjamin Gaignard 0 siblings, 1 reply; 12+ messages in thread From: Mark Brown @ 2017-11-02 11:10 UTC (permalink / raw) To: Greg KH Cc: Laura Abbott, Benjamin Gaignard, sumit.semwal, arve, riandrews, dan.carpenter, devel, linux-kernel, dri-devel, linux-api [-- Attachment #1: Type: text/plain, Size: 1129 bytes --] On Thu, Nov 02, 2017 at 11:44:07AM +0100, Greg KH wrote: > On Tue, Oct 31, 2017 at 07:11:53PM +0000, Mark Brown wrote: > > There was a discussion a while ago in the context of I2C/SPI MFDs > > which concluded that if you need a bus and it's going to be effectively > > noop then you should just use the platform bus as anything else will > > consist almost entirely of cut'n'paste from the platform bus with some > > light sed usage and code duplication is bad. It's not super lovely as > > it's not actually a memory mapped device but it's the best idea we've > > got. > Ugh, I hate that. What's wrong with using a "virtual" device instead? It was the duplication, initially everyone was making buses. > I can create a "virtual" bus for things like this if they really want a > "simple" bus, abusing platform for this is the major reason I hate the > platform bus code... In the MFD case they're physical devices, they're just usually on the wrong side of an I2C or SPI link. Plus MFD already handles platform devices for things that are memory mapped so it's a bit of a more natural fit there. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6 2/2] staging: ion: create one device entry per heap 2017-11-02 11:10 ` Mark Brown @ 2017-11-06 14:42 ` Benjamin Gaignard 2017-11-06 14:46 ` Greg KH 0 siblings, 1 reply; 12+ messages in thread From: Benjamin Gaignard @ 2017-11-06 14:42 UTC (permalink / raw) To: Mark Brown Cc: Greg KH, Laura Abbott, Sumit Semwal, Arve Hjønnevåg, Riley Andrews, Dan Carpenter, driverdevel, Linux Kernel Mailing List, dri-devel@lists.freedesktop.org, linux-api 2017-11-02 12:10 GMT+01:00 Mark Brown <broonie@kernel.org>: > On Thu, Nov 02, 2017 at 11:44:07AM +0100, Greg KH wrote: >> On Tue, Oct 31, 2017 at 07:11:53PM +0000, Mark Brown wrote: > >> > There was a discussion a while ago in the context of I2C/SPI MFDs >> > which concluded that if you need a bus and it's going to be effectively >> > noop then you should just use the platform bus as anything else will >> > consist almost entirely of cut'n'paste from the platform bus with some >> > light sed usage and code duplication is bad. It's not super lovely as >> > it's not actually a memory mapped device but it's the best idea we've >> > got. > >> Ugh, I hate that. What's wrong with using a "virtual" device instead? > > It was the duplication, initially everyone was making buses. > >> I can create a "virtual" bus for things like this if they really want a >> "simple" bus, abusing platform for this is the major reason I hate the >> platform bus code... > > In the MFD case they're physical devices, they're just usually on the > wrong side of an I2C or SPI link. Plus MFD already handles platform > devices for things that are memory mapped so it's a bit of a more > natural fit there. What I can do is to register an ion bus (like cec one for example), add one ion parent device so heaps will appear in /sys/bus/ion/ion* and /sys/devices/ion/ion* Does that could sound good enough ? Benjamin ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v6 2/2] staging: ion: create one device entry per heap 2017-11-06 14:42 ` Benjamin Gaignard @ 2017-11-06 14:46 ` Greg KH 0 siblings, 0 replies; 12+ messages in thread From: Greg KH @ 2017-11-06 14:46 UTC (permalink / raw) To: Benjamin Gaignard Cc: Mark Brown, Laura Abbott, Sumit Semwal, Arve Hjønnevåg, Riley Andrews, Dan Carpenter, driverdevel, Linux Kernel Mailing List, dri-devel@lists.freedesktop.org, linux-api On Mon, Nov 06, 2017 at 03:42:04PM +0100, Benjamin Gaignard wrote: > 2017-11-02 12:10 GMT+01:00 Mark Brown <broonie@kernel.org>: > > On Thu, Nov 02, 2017 at 11:44:07AM +0100, Greg KH wrote: > >> On Tue, Oct 31, 2017 at 07:11:53PM +0000, Mark Brown wrote: > > > >> > There was a discussion a while ago in the context of I2C/SPI MFDs > >> > which concluded that if you need a bus and it's going to be effectively > >> > noop then you should just use the platform bus as anything else will > >> > consist almost entirely of cut'n'paste from the platform bus with some > >> > light sed usage and code duplication is bad. It's not super lovely as > >> > it's not actually a memory mapped device but it's the best idea we've > >> > got. > > > >> Ugh, I hate that. What's wrong with using a "virtual" device instead? > > > > It was the duplication, initially everyone was making buses. > > > >> I can create a "virtual" bus for things like this if they really want a > >> "simple" bus, abusing platform for this is the major reason I hate the > >> platform bus code... > > > > In the MFD case they're physical devices, they're just usually on the > > wrong side of an I2C or SPI link. Plus MFD already handles platform > > devices for things that are memory mapped so it's a bit of a more > > natural fit there. > > What I can do is to register an ion bus (like cec one for example), > add one ion parent device so heaps will appear in /sys/bus/ion/ion* > and /sys/devices/ion/ion* > > Does that could sound good enough ? I would like to see that... thanks, greg k-h ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v6 0/2] staging: ion: get one device per heap @ 2017-11-06 15:59 Benjamin Gaignard 0 siblings, 0 replies; 12+ messages in thread From: Benjamin Gaignard @ 2017-11-06 15:59 UTC (permalink / raw) To: labbott, sumit.semwal, gregkh, arve, riandrews, broonie, dan.carpenter Cc: devel, linux-kernel, dri-devel, linux-api, Benjamin Gaignard version 6: - add an ION bus so heap are show as devices in /sys/bus/ion/ instead of platform bus. - split the patch in two: one for include reordering and one for per heap device change - rebased on top of next-2017110 tag version 5: - create a configuration flag to keep legacy Ion misc device version 4: - add a configuration flag to switch between legacy Ion misc device and one device per heap version. This change has been suggested after Laura talks at XDC 2017. 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. Using legacy Ion misc device is still by setting ION_LEGACY_DEVICE_API configuration flag. Benjamin Gaignard (2): staging: ion: reorder include staging: ion: create one device entry per heap drivers/staging/android/TODO | 1 - drivers/staging/android/ion/Kconfig | 7 +++ drivers/staging/android/ion/ion-ioctl.c | 18 +++++++- drivers/staging/android/ion/ion.c | 76 +++++++++++++++++++++++++++------ drivers/staging/android/ion/ion.h | 15 ++++++- 5 files changed, 98 insertions(+), 19 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-11-06 15:59 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-10-23 15:55 [PATCH v6 0/2] staging: ion: get one device per heap Benjamin Gaignard 2017-10-23 15:55 ` [PATCH v6 1/2] staging: ion: simplify ioctl args checking function Benjamin Gaignard 2017-10-23 15:55 ` [PATCH v6 2/2] staging: ion: create one device entry per heap Benjamin Gaignard 2017-10-24 16:14 ` Jordan Crouse 2017-10-31 19:03 ` Laura Abbott 2017-10-31 19:11 ` Mark Brown 2017-10-31 19:45 ` Laura Abbott 2017-11-02 10:44 ` Greg KH 2017-11-02 11:10 ` Mark Brown 2017-11-06 14:42 ` Benjamin Gaignard 2017-11-06 14:46 ` Greg KH -- strict thread matches above, loose matches on Subject: below -- 2017-11-06 15:59 [PATCH v6 0/2] staging: ion: get one device " Benjamin Gaignard
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).